Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacingrq_for_each_bio

[Satyam Sharma]

On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:

> On Tue, 21 Aug 2007, Satyam Sharma wrote:
> > 
> > This is turning rather funny :-)
> > 
> > * Why the printk() conversion specifier must be "%llu"?
> > 
> > When reusing parts of this code (such as this debug message) for another
> > 32-bit driver (we certainly seem to care about this, as per your reply),
> > the largest size of sector_t can be "unsigned long long", thereby causing
> > truncated output, and the following warning:
> > 
> > warning: format ???lu???expects type ???ong unsigned int??? but argument 2 has
> > type ???ector_t?
> 
> You will get a warning, so you will know.

Ah. So you'd much rather prefer that code in drivers/ make assumptions:
sizeof(long) == sizeof(long long), and, sizeof(long) == sizeof(sector_t),
hmmm?


> > Therefore, let us not depend on the fact that this driver is being used
> > only for 64-bit platforms as of now (especially since this is in drivers/
> > and not in arch/) and rather make the printk() specifier as "%llu".
> > 
> > [ Sadly, I had to repeat most of my previous mail, which Jan Engelhardt
> > appears to have snipped. ]
> 
> [ and you dropped the cell mailing list from the CC list ]

I don't enjoy getting "this is a subscriber-only list" notifications,
thank you.


> > * Why we require an explicit (unsigned long long) cast?
> > 
> > Having made the above change (and say we don't have an explicit cast
> > there), that line now becomes:
> > 
> > 	printk("... %llu\n", ..., iter.bio->bi_sector);
> > 
> > Now if we build this code with CONFIG_LBD=n, sector_t becomes just an
> > "unsigned long" (whereas printk specifier is the larger "%llu") thereby
> > causing:
> > 
> > warning: format ???llu???expects type ???ong long unsigned int??? but argument
> > 2 has type ???ector_t?
> > 
> > Therefore, let us shut this up with an explicit (unsigned long long) cast,
> > as is done in most other existing code in the kernel where we want to get
> > bi_sector printed out, which would correctly convert the value to the
> > larger integer type (even negative ones, due to sign-extension) and print
> > it out.
> > 
> > > On the other hand, adding a cast may hide bugs:
> > >   - cast to unsigned long long: When sector_t is changed to an even larger
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >     type, it will be silently truncated as well.
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > IMHO, this is not a valid enough reason, given the above (BTW if/when that
> > happens, we'll have to update the printk conversion specifer as well).
> > 
> > Personally, I'd say code that assumes "sizeof(unsigned long long) ==
> > sizeof(unsigned long)", and also that assumes "sizeof(unsigned long) ==
> > sizeof(sector_t)" -- both assumptions _are_ made by the above code --
> > is not good taste, even if both may be true for PS3 platform. So unless
> > we decide "nobody except that platform would ever build this driver
> > anyway", I'd suggest to make the printk specifier as "%llu" and use an
> > explicit (unsigned long long) cast. Therefore (on top of this series):
> 
> Adding such a cast makes it impossible for the compiler to detect (and warn us)
> if something has changed.

Change as in increase in size, right? Did you even read the mail? The only
argument you had given against the cast to (unsigned long long) was what
has been underlined above, which was bogus, considering we would have to
change the "%lu" specifier in the printk() also, when that happens
(otherwise suffer silent truncation).

But looks like you /are/ deciding "nobody except PS3 platform would ever
build this driver anyway" ... so okay, the current code, will all it's
non-portable assumptions, is okay.

Cheers,

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/