| Message ID | 005b6680-da19-495a-bc99-9ec3f66a5e74@p183 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] block, rust: simplify validate_block_size() function | expand |
On 31.08.24 22:15, Alexey Dobriyan wrote: > Using range and contains() method is just fancy shmancy way of writing This language doesn't fit into a commit message. Please give a technical reason to change this. > two comparisons. Using range doesn't prevent any bugs here because > typing "=" in range can be forgotten just as easily as in "<=" operator. I don't think that using traditional comparisons is an improvement. When using `contains`, both of the bounds are visible with one look. When using two comparisons, you have to first parse that they compare the same variable and then look at the bounds. > Also delete few comments of "increment i by 1" variety. As Miguel already said, these are part of the documentation. Do not remove them. --- Cheers, Benno
On Sat, Aug 31, 2024 at 08:39:45PM +0000, Benno Lossin wrote: > On 31.08.24 22:15, Alexey Dobriyan wrote: > > Using range and contains() method is just fancy shmancy way of writing > > This language doesn't fit into a commit message. Please give a technical > reason to change this. Oh come on! > > two comparisons. Using range doesn't prevent any bugs here because > > typing "=" in range can be forgotten just as easily as in "<=" operator. > > I don't think that using traditional comparisons is an improvement. They are an improvement, or rather contains() on integers is of dubious value. First, coding style mandates that there are no whitespace on both sides of "..". This merges all characters into one Perl-like line noise. Second, when writing C I've a habit of writing comparisons like numeric line in school which goes from left to right: 512 ... size .. PAGE_SIZE ------> infinity See? Now it is easy to insert comparisons: 512 <= size <= PAGE_SIZE Of course in C the middle variable must be duplicated but so what? How hard is to parse this? 512 <= size && size <= PAGE_SIZE And thirdly, to a C/C++ dev, passing u32 by reference instead of by value to a function which obviously doesn't mutate it screams WHAT??? > When > using `contains`, both of the bounds are visible with one look. Yes, they are within 4 characters of each other 2 of which are whitespace. This is what this patch is all about: contains() for integers? I can understand contains() instead of strstr() but for integers? > When > using two comparisons, you have to first parse that they compare the > same variable and then look at the bounds. Yes but now you have to parse () and .. and &. > > Also delete few comments of "increment i by 1" variety. > > As Miguel already said, these are part of the documentation. Do not > remove them. Kernel has its fair share of 1:1 kernel-doc comments which contain exactly zero useful information because everything is in function signature already. This is the original function: /* blk_validate_limits() validates bsize, so drivers don't usually need to */ static inline int blk_validate_block_size(unsigned long bsize) { if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize)) return -EINVAL; return 0; } I have to say this is useful comment because it refers to another more complex function and hints that it should be used instead. It doesn't reiterate what the function does internally. Something was lost in translation.
On 01.09.24 21:56, Alexey Dobriyan wrote: > On Sat, Aug 31, 2024 at 08:39:45PM +0000, Benno Lossin wrote: >> On 31.08.24 22:15, Alexey Dobriyan wrote: >>> two comparisons. Using range doesn't prevent any bugs here because >>> typing "=" in range can be forgotten just as easily as in "<=" operator. >> >> I don't think that using traditional comparisons is an improvement. > > They are an improvement, or rather contains() on integers is of dubious > value. > > First, coding style mandates that there are no whitespace on both sides > of "..". This merges all characters into one Perl-like line noise. Can you please provide a link to that coding style? This is the default formatting of rustfmt. > Second, when writing C I've a habit of writing comparisons like numeric > line in school which goes from left to right: > > 512 ... size .. PAGE_SIZE ------> infinity > > See? > Now it is easy to insert comparisons: > > 512 <= size <= PAGE_SIZE > > Of course in C the middle variable must be duplicated but so what? > > How hard is to parse this? > > 512 <= size && size <= PAGE_SIZE I would argue that this is just a matter of taste and familiarity. In mathematics there are also several other ways to express the above. For example $size \in [512, PAGE_SIZE]$. > And thirdly, to a C/C++ dev, passing u32 by reference instead of by > value to a function which obviously doesn't mutate it screams WHAT??? That is because the function is implemented with generics over the type inside of the range. So if you have your own type, you can use `..` and `..=` to create ranges. For big structs you wouldn't use by-value even in C and C++. >> When >> using `contains`, both of the bounds are visible with one look. > > Yes, they are within 4 characters of each other 2 of which are > whitespace. > > This is what this patch is all about: contains() for integers? > I can understand contains() instead of strstr() but for integers? Don't get me wrong, we can have a discussion about this. I just think that it is not as clear-cut as you make it out to be. >> When >> using two comparisons, you have to first parse that they compare the >> same variable and then look at the bounds. > > Yes but now you have to parse () and .. and &. As I said above, it's a matter of familiarity. I don't think that using contains is inherently worse. >>> Also delete few comments of "increment i by 1" variety. >> >> As Miguel already said, these are part of the documentation. Do not >> remove them. > > Kernel has its fair share of 1:1 kernel-doc comments which contain > exactly zero useful information because everything is in function > signature already. Sure, but in this case, I don't think everything is in the function signature. > This is the original function: > > /* blk_validate_limits() validates bsize, so drivers don't usually need to */ > static inline int blk_validate_block_size(unsigned long bsize) > { > if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize)) > return -EINVAL; > return 0; > } > > I have to say this is useful comment because it refers to another more > complex function and hints that it should be used instead. It doesn't > reiterate what the function does internally. > > Something was lost in translation. We currently don't have that function in Rust. Also note that in contrast to C, this function is private. So no driver is able to call it anyways. In this case, documentation is not really necessary, as we only require it for publicly facing functions. But for those I think it is useful to reiterate what the code does. They might be the entrypoint for first time kernel developers and giving them an easier time of understanding is IMO a good thing. --- Cheers, Benno
--- a/rust/kernel/block/mq/gen_disk.rs +++ b/rust/kernel/block/mq/gen_disk.rs @@ -43,21 +43,16 @@ pub fn rotational(mut self, rotational: bool) -> Self { self } - /// Validate block size by verifying that it is between 512 and `PAGE_SIZE`, - /// and that it is a power of two. fn validate_block_size(size: u32) -> Result<()> { - if !(512..=bindings::PAGE_SIZE as u32).contains(&size) || !size.is_power_of_two() { - Err(error::code::EINVAL) - } else { + if 512 <= size && size <= bindings::PAGE_SIZE as u32 && size.is_power_of_two() { Ok(()) + } else { + Err(error::code::EINVAL) } } /// Set the logical block size of the device to be built. /// - /// This method will check that block size is a power of two and between 512 - /// and 4096. If not, an error is returned and the block size is not set. - /// /// This is the smallest unit the storage device can address. It is /// typically 4096 bytes. pub fn logical_block_size(mut self, block_size: u32) -> Result<Self> { @@ -68,9 +63,6 @@ pub fn logical_block_size(mut self, block_size: u32) -> Result<Self> { /// Set the physical block size of the device to be built. /// - /// This method will check that block size is a power of two and between 512 - /// and 4096. If not, an error is returned and the block size is not set. - /// /// This is the smallest unit a physical storage device can write /// atomically. It is usually the same as the logical block size but may be /// bigger. One example is SATA drives with 4096 byte physical block size
Using range and contains() method is just fancy shmancy way of writing two comparisons. Using range doesn't prevent any bugs here because typing "=" in range can be forgotten just as easily as in "<=" operator. Also delete few comments of "increment i by 1" variety. Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> --- rust/kernel/block/mq/gen_disk.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)