While inspecting the qcow2 format code, I was checking whether it was going to correctly support 64-bit filesystems. It looks like the qcow2 format itself supports 64-bit filesystems, which is good, but I also found inconsistent use of types within libext2fs for block numbers, file offsets, groups, inodes, etc.
There are uses of blk64_t, ext2_off64_t, long long, (unsigned) long, etc. in the libext2fs code. The currently defined types are:
typedef __u32 blk_t;
typedef __u64 blk64_t;
typedef __u32 dgrp_t;
typedef __u32 ext2_off_t;
typedef __u64 ext2_off64_t;
typedef __s64 e2_blkcnt_t;
It would be nice to get some consistent naming for these types, like ext2_blk_t, ext2_blk64_t (or possibly ext2_fsblk_t to match the kernel), ext2_group_t, and ext2_blkcnt_t (this appears to be negative in some places, so isn't easily replaced with ext2_blk64_t.
For example, io_channel_read_blk64() uses "long long", while both ext2fs_{read,write}_inode_full() _incorrectly_ use "unsigned long". I guess this wasn't noticed because > 16TB isn't directly usable on 32-bit Linux due to the block device limits, but it doesn't seem right to leave it as-is.
I understand it would be a lot of code churn, but I think it would be valuable to fix up the types used in e2fsprogs in order to clarify usage and remove errors, just like we did with the kernel code. Is there any interest in accepting patches like this upstream? Both ext2fs_{read,write}_inode_full() need to be fixed, and I expect even more places will need fixing.
Cheers, Andreas
Cheers, Andreas
On Mon, May 16, 2011 at 03:55:54PM -0600, Andreas Dilger wrote:
>
> There are uses of blk64_t, ext2_off64_t, long long, (unsigned) long,
> etc. in the libext2fs code. The currently defined types are:
>
> typedef __u32 blk_t;
> typedef __u64 blk64_t;
> typedef __u32 dgrp_t;
> typedef __u32 ext2_off_t;
> typedef __u64 ext2_off64_t;
> typedef __s64 e2_blkcnt_t;
>
> It would be nice to get some consistent naming for these types, like
> ext2_blk_t, ext2_blk64_t (or possibly ext2_fsblk_t to match the
> kernel), ext2_group_t, and ext2_blkcnt_t (this appears to be
> negative in some places, so isn't easily replaced with ext2_blk64_t.
You're raising two issues here. One is the fact that the names of the
types aren't completely consistent. Yes, that's true, but I'm not
entirely convinced the code churn in renaming all of the types is worth
the pain. That to me is purely aesthetics.
Note, by the way, that e2_blkcnt_t is quite different from blk64_t;
the first is used for logical block numbers (and we use negative
numbers to signal indirect blocks), where as blk64_t is used for
physical block numbers. We have two different types in the kernel for
these two concept, as well.
> For example, io_channel_read_blk64() uses "long long", while both
> ext2fs_{read,write}_inode_full() _incorrectly_ use "unsigned long".
> I guess this wasn't noticed because > 16TB isn't directly usable on
> 32-bit Linux due to the block device limits, but it doesn't seem
> right to leave it as-is.
Well, that's not an example, a big (at least in the case of
ext2fs_{read,write}_inode_full(). They should be using blk64_t
instead.
The reason why the io_* functions use long long was because
conceptually they're a HAL that doesn't was considered logically
separate from the ext2fs library. So they don't #include ext2_fs.h,
which is why they use unsigned long long instead of blk64_t.
> I understand it would be a lot of code churn, but I think it would
> be valuable to fix up the types used in e2fsprogs in order to
> clarify usage and remove errors, just like we did with the kernel
> code. Is there any interest in accepting patches like this
> upstream? Both ext2fs_{read,write}_inode_full() need to be fixed,
> and I expect even more places will need fixing.
I'm not convinced that a mass renaming of all the types is really
necessary to find these sorts of bugs. A human has to look at the
types and decide whether they are correct; renaming the types via a
global search and replace isn't going to help find them....
- Ted
On May 16, 2011, at 19:16, Ted Ts'o wrote:
> On Mon, May 16, 2011 at 03:55:54PM -0600, Andreas Dilger wrote:
>>
>> There are uses of blk64_t, ext2_off64_t, long long, (unsigned) long,
>> etc. in the libext2fs code. The currently defined types are:
>>
>> typedef __u32 blk_t;
>> typedef __u64 blk64_t;
>> typedef __u32 dgrp_t;
>> typedef __u32 ext2_off_t;
>> typedef __u64 ext2_off64_t;
>> typedef __s64 e2_blkcnt_t;
>>
>> It would be nice to get some consistent naming for these types, like
>> ext2_blk_t, ext2_blk64_t (or possibly ext2_fsblk_t to match the
>> kernel), ext2_group_t, and ext2_blkcnt_t (this appears to be
>> negative in some places, so isn't easily replaced with ext2_blk64_t.
>
> You're raising two issues here. One is the fact that the names of the
> types aren't completely consistent. Yes, that's true, but I'm not
> entirely convinced the code churn in renaming all of the types is worth
> the pain. That to me is purely aesthetics.
It is partly aesthetics, but it also relates to making the code logic
easier to follow, and also being able to more easily determine if the
code is actually correct.
> Note, by the way, that e2_blkcnt_t is quite different from blk64_t;
> the first is used for logical block numbers (and we use negative
> numbers to signal indirect blocks), where as blk64_t is used for
> physical block numbers.
Using a better type name, like "ext2_logblk_t" to distinguish it from
"ext2_fsblk_t" would make that distinction more clear, IMHO.
> We have two different types in the kernel for these two concept, as well.
>
>> For example, io_channel_read_blk64() uses "long long", while both
>> ext2fs_{read,write}_inode_full() _incorrectly_ use "unsigned long".
>> I guess this wasn't noticed because > 16TB isn't directly usable on
>> 32-bit Linux due to the block device limits, but it doesn't seem
>> right to leave it as-is.
>
> Well, that's not an example, a big (at least in the case of
> ext2fs_{read,write}_inode_full(). They should be using blk64_t
> instead.
>
> The reason why the io_* functions use long long was because
> conceptually they're a HAL that doesn't was considered logically
> separate from the ext2fs library. So they don't #include ext2_fs.h,
> which is why they use unsigned long long instead of blk64_t.
OK, I can accept this.
>> I understand it would be a lot of code churn, but I think it would
>> be valuable to fix up the types used in e2fsprogs in order to
>> clarify usage and remove errors, just like we did with the kernel
>> code. Is there any interest in accepting patches like this
>> upstream? Both ext2fs_{read,write}_inode_full() need to be fixed,
>> and I expect even more places will need fixing.
>
> I'm not convinced that a mass renaming of all the types is really
> necessary to find these sorts of bugs. A human has to look at the
> types and decide whether they are correct; renaming the types via a
> global search and replace isn't going to help find them....
I'm not suggesting a global search & replace of "long long" or anything.
However, the current mish-mash of int vs. long vs. dgrp_t for group numbers,
__u64 vs blk64_t, etc doesn't make it clear when something is intentionally
that type, or just happens to be working for now. Having separate types
for groups vs. physical blocks vs. logical blocks as we do in the kernel
will improve the quality of the code itself, I think.
Cheers, Andreas
> I'm not suggesting a global search & replace of "long long" or anything.
> However, the current mish-mash of int vs. long vs. dgrp_t for group numbers,
> __u64 vs blk64_t, etc doesn't make it clear when something is intentionally
> that type, or just happens to be working for now. Having separate types
> for groups vs. physical blocks vs. logical blocks as we do in the kernel
> will improve the quality of the code itself, I think.
>
> Cheers, Andreas
>
I have to agree with Andreas here, types in e2fsprogs are not consistent
and I have encounter this several times, trying to decide which one is
the best, looking for examples, and finding different uses in different
parts of e2fsprogs tools. It is a mess. I think it is worth the core crunch
I someone is willing to do the job.
Thanks!
-Lukas
On May 17, 2011, at 1:04 AM, Andreas Dilger wrote:
> I'm not suggesting a global search & replace of "long long" or anything.
> However, the current mish-mash of int vs. long vs. dgrp_t for group numbers,
> __u64 vs blk64_t, etc doesn't make it clear when something is intentionally
> that type, or just happens to be working for now. Having separate types
> for groups vs. physical blocks vs. logical blocks as we do in the kernel
> will improve the quality of the code itself, I think.
But you are talking about doing a global search and replace of "e2_blkcnt_t"
for something else like "ext2_logblk_t", aren't you? If we need to better
document all of the types, yes, that will probably help.
But I don't see how a global search and replace of "dgrp_t" for "ext2_group_t"
is going to help us find the places where a group number was assigned to
an int.
Yes, the naming scheme is inconsistent. e2fsprogs is a very old code base,
and many of these decisions were made a very long time ago. But I don't
see how a global search and replace of one typedef name for another will
_find _bugs_. It won't help in the cases where we used a raw type, and it
won't help where we accidentally used the wrong typedef'ed name.
It might help a new comer to the code base when they are writing new code,
yes. So would better documentation. Against that we have to weigh
the cost of the code churn, and the fact that patches to the maint branch
won't be easily pulled to master, etc.
-- Ted
On 5/17/11 12:04 AM, Andreas Dilger wrote:
> On May 16, 2011, at 19:16, Ted Ts'o wrote:
>> On Mon, May 16, 2011 at 03:55:54PM -0600, Andreas Dilger wrote:
>>>
>>> There are uses of blk64_t, ext2_off64_t, long long, (unsigned) long,
>>> etc. in the libext2fs code. The currently defined types are:
>>>
>>> typedef __u32 blk_t;
>>> typedef __u64 blk64_t;
>>> typedef __u32 dgrp_t;
>>> typedef __u32 ext2_off_t;
>>> typedef __u64 ext2_off64_t;
>>> typedef __s64 e2_blkcnt_t;
>>>
>>> It would be nice to get some consistent naming for these types, like
>>> ext2_blk_t, ext2_blk64_t (or possibly ext2_fsblk_t to match the
>>> kernel), ext2_group_t, and ext2_blkcnt_t (this appears to be
>>> negative in some places, so isn't easily replaced with ext2_blk64_t.
>>
>> You're raising two issues here. One is the fact that the names of the
>> types aren't completely consistent. Yes, that's true, but I'm not
>> entirely convinced the code churn in renaming all of the types is worth
>> the pain. That to me is purely aesthetics.
>
> It is partly aesthetics, but it also relates to making the code logic
> easier to follow, and also being able to more easily determine if the
> code is actually correct.
>
>> Note, by the way, that e2_blkcnt_t is quite different from blk64_t;
>> the first is used for logical block numbers (and we use negative
>> numbers to signal indirect blocks), where as blk64_t is used for
>> physical block numbers.
>
> Using a better type name, like "ext2_logblk_t" to distinguish it from
> "ext2_fsblk_t" would make that distinction more clear, IMHO.
If you're going for consistency that'd be "ext2_lblk_t" I think.
(logblk differs from the kernel and might imply something about the log itself...)
I too agree that better type consistency would help. When I fixed the signed
block containers way back when, it was Not Fun searching through the mishmash of
apparently random type selections. And having "int blocknr," besides often
being wrong, doesn't make it obvious if you're talking about a physical block,
a logical block, a block offset within a group, or what.
Having typedefs doesn't necessarily enforce correctness but it makes it
easier to get right, IMHO.
-Eric
On May 17, 2011, at 04:36, Theodore Tso wrote:
> On May 17, 2011, at 1:04 AM, Andreas Dilger wrote:
>> I'm not suggesting a global search & replace of "long long" or anything.
>> However, the current mish-mash of int vs. long vs. dgrp_t for group numbers,
>> __u64 vs blk64_t, etc doesn't make it clear when something is intentionally
>> that type, or just happens to be working for now. Having separate types
>> for groups vs. physical blocks vs. logical blocks as we do in the kernel
>> will improve the quality of the code itself, I think.
>
> But you are talking about doing a global search and replace of "e2_blkcnt_t"
> for something else like "ext2_logblk_t", aren't you? If we need to better
> document all of the types, yes, that will probably help.
Consistency is one of the important reasons. Even though I've been an e2fsprogs contributor for many years already, I struggle to remember the right type to use, because there is neither consistency in the naming, nor consistency in the code about the usage. I think the cleanup that was done in the ext4 code for the naming helped the readability of that code considerably.
> But I don't see how a global search and replace of "dgrp_t" for "ext2_group_t"
> is going to help us find the places where a group number was assigned to
> an int.
That is bypassing my point. My intent is not only to start using a consistent naming scheme for existing types, but also to go through the code for places that do not use consistent types (e.g. int, long, ...) and replace those with the appropriate type as well.
> Yes, the naming scheme is inconsistent. e2fsprogs is a very old code base,
> and many of these decisions were made a very long time ago.
Sure, no slight is intended over the current state of the code. The e2fsprogs code base has definitely been a good example (especially the regression tests make it stand above the crowd, IMHO). However, I think the intervening years have also taught us more about making code cleaner and more easily maintained.
> But I don't see how a global search and replace of one typedef name for
> another will _find _bugs_. It won't help in the cases where we used a raw
> type, and it won't help where we accidentally used the wrong typedef'ed name.
Sure, just renaming the types won't remove any existing bugs, but it will help prevent bugs in the future because the people that are reading and modifying the code will understand it better. It will be more clear that when a function takes an ext2_fsblk_t as a parameter that this is an absolute block number, and when it takes ext2_lblk_t (as Eric suggests) it is a file logical block, etc. It will also help ensure that developers choose the right size of variable for the value being manipulated, and hopefully avoid truncation errors for > 2^32-block filesystems, which we are very interested to support. The availability of 3TB drives in our standard 8+2 RAID-6 create a 24TB LUN, and 4TB drives are probably going to be available within a year.
> It might help a new comer to the code base when they are writing new code,
> yes. So would better documentation. Against that we have to weigh
> the cost of the code churn, and the fact that patches to the maint branch
> won't be easily pulled to master, etc.
I agree - there is some added maintenance effort for a short time, but I think the long-term benefits are also valuable (in the form of fewer bugs). Also, I don't expect that I'll have this kind of patch available overnight. The search-and-replace is easy, but actually finding bugs will be the hard/slow part, but it needs to start somewhere.
My only concern would be about compatibility of the external interface. Since only the type names are changing and not the field size, there shouldn't be any binary compatibility problems. I worry about things like C++ and coff/elf libraries that might do function name mangling to try and get a stable binary interface. It may be that they will have problems because of the name change, even though the types are remaining the same size.
Cheers, Andreas