Migrate quota module to libc FFI types #750
Conversation
Susurrus
force-pushed
the
quota_ffi
branch
2 times, most recently
from
b8f4007 to
1117842
Compare
Susurrus
changed the title
|
Need to add setters for the various quota limits and also add some examples: 1 retrieving the quota limits, one enabling and settings limits, and one modifying limits. |
Susurrus
changed the title
|
I don't see why this code shouldn't also work across all Linux, rather than just |
|
This is ready for review if someone's up for it. |
src/sys/quota.rs
Outdated
| pub type QuotaSubCmd = c_int; | ||
| impl QuotaCmd { | ||
| fn as_int(&self) -> c_int { | ||
| (((self.0 as i32) << 8) | ((self.1 as i32) & 0x00ff)) as c_int |
There was a problem hiding this comment.
I don't think this will work. The QuotaSubCmd values are constants, defined by libc with values like 0x100, 0x200, etc. So you should eliminate the shift.
There was a problem hiding this comment.
This was in there already, so I actually didn't take a look at it at all! But the implementation follows from the QCMD() macro in /usr/include/linux/quota.h, so I'm not certain what the problem is. on Linux QSYNC for example is a 24-bit value, we shift it up 8 bits and then or-in a 2 bit value at the bottom. That seems fine to me. What are you looking at that makes you think this is incorrect?
I actually think we should block until QCMD is added to libc and actually make this rely on that instead.
There was a problem hiding this comment.
Sorry, I was looking at the BSD definitions for quota commands, which are also similar to the Linux definitions when _LINUX_QUOTA_VERSION < 2
| QIF_TIMES; | ||
| QIF_ALL; | ||
| /// Set the absolute limit on disk quota blocks allocated. | ||
| pub fn set_blocks_hard_limit(&mut self, limit: u64) { |
There was a problem hiding this comment.
Should this method also set QIF_BLIMITS?
There was a problem hiding this comment.
I don't think so. If anything, it should unset it. This API basically has two modes: reading the current state and modifying the existing state. The QIF_ values are used to indicate which values are set in both cases. So in case you read, modify, then write, we actually want to completely ignore the value set in the struct, as it might be left-over from a read, and instead force the user to specify it explicitly for this specific write operation. That way there's no confusion.
I think we could simplify the API by doing something like what you suggest but actually use separate fields to track reading and writing and then setting the writing_valid_fields field to the proper flags when any of the appropriate set_* functions are called might work. Thoughts?
|
Waiting on rust-lang/libc#747 to resolve Linux/MIPS build errors. |
|
Would it be possible to add some tests? |
|
@polachok You originally added this API, do you have any tests or examples that you could possibly volunteer here so we could expand a test suite for this API? I'm not too familiar with this API, so would appreciate any input from someone more knowledgable. @asomers I already pinged @polachok via email yesterday asking this same thing basically and haven't heard anything, so I dunno if we'll get a response. I'd prefer to merge this and come back and extend our test suite later. i don't really want to spend the time to dive deep into this API know when my priority is closing #748. |
|
Sorry for late response, I haven't got a chance to use the API after adding it (besides basic tests I don't have know), so I'm ok with whatever you have. We can fix it later if there's something wrong. |
|
Fine? No. I'm slightly depressed whenever I add untested code. But your changes don't add new code so much as refactor existing code, so I guess it's ok. bors r+ |
|
Yeah, it's definitely not ideal. I'd like to vastly improve our test coverage (I'd actually like to look at The next step as part of my work towards a 1.0 release will be to get every subsystem testable (though I dunno how much will require root). |
No description provided.