LinuxLists.cc - [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

On Thu, 16 Aug 2007, NeilBrown wrote:
> Every usage of rq_for_each_bio wraps a usage of
> bio_for_each_segment, so these can be combined into
> rq_for_each_segment.
>
> We define "struct req_iterator" to hold the 'bio' and 'index' that
> are needed for the double iteration.

> --- .prev/drivers/block/ps3disk.c 2007-08-16 20:37:26.000000000 +1000
> +++ ./drivers/block/ps3disk.c 2007-08-16 20:50:07.000000000 +1000
> @@ -91,30 +91,30 @@ static void ps3disk_scatter_gather(struc
> struct request *req, int gather)
> {
> unsigned int offset = 0;
> - struct bio *bio;
> - sector_t sector;
> + struct req_iterator iter;
> struct bio_vec *bvec;
> - unsigned int i = 0, j;
> + unsigned int i = 0;
> size_t size;
> void *buf;
>
> - rq_for_each_bio(bio, req) {
> - sector = bio->bi_sector;
> + rq_for_each_segment(bvec, req, iter) {
> + unsigned long flags;
> dev_dbg(&dev->sbd.core,
> "%s:%u: bio %u: %u segs %u sectors from %lu\n",
> - __func__, __LINE__, i, bio_segments(bio),
> - bio_sectors(bio), sector);
> - bio_for_each_segment(bvec, bio, j) {
> + __func__, __LINE__, i, bio_segments(iter.bio),
> + bio_sectors(iter.bio),
> + (unsigned long)iter.bio->bi_sector);
^^^^^^^^^^^^^^^
Superfluous cast: PS3 is 64-bit only, and casts are evil.

> +
> size = bvec->bv_len;
> - buf = __bio_kmap_atomic(bio, j, KM_IRQ0);
> + buf = bvec_kmap_irq(bvec, flags);

As you already mentioned in the preample of the patch series, this changes
functionality.

> if (gather)
> memcpy(dev->bounce_buf+offset, buf, size);
> else
> memcpy(buf, dev->bounce_buf+offset, size);
> offset += size;
> flush_kernel_dcache_page(bio_iovec_idx(bio, j)->bv_page);
> - __bio_kunmap_atomic(bio, KM_IRQ0);
> - }
> + bio_kunmap_bvec(bvec, flags);
> +
> i++;
> }
> }

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

2007-08-18 14:25:00

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:

> On Thu, 16 Aug 2007, NeilBrown wrote:
> [...]
> > dev_dbg(&dev->sbd.core,
> > "%s:%u: bio %u: %u segs %u sectors from %lu\n",
> > - __func__, __LINE__, i, bio_segments(bio),
> > - bio_sectors(bio), sector);
> > - bio_for_each_segment(bvec, bio, j) {
> > + __func__, __LINE__, i, bio_segments(iter.bio),
> > + bio_sectors(iter.bio),
> > + (unsigned long)iter.bio->bi_sector);
> ^^^^^^^^^^^^^^^
> Superfluous cast: PS3 is 64-bit only, and casts are evil.

(Sorry for butting in), but I wonder if relying on that in code here
(granted, a PS3-only driver, but it's in drivers/ and not arch/) would
be good style. Why not just print it out as: "%llu" and use an (unsigned
long long) cast, considering that's the largest type sector_t can ever
have (irrespective of arch) ... I can see most of the other generic places
in the kernel (such as in drivers/md/) also using the latter (%llu with
unsigned long long cast) to get bi_sector printed.

2007-08-18 14:30:57

by Jan Engelhardt

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

On Aug 18 2007 20:07, Satyam Sharma wrote:
>On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
>
>> On Thu, 16 Aug 2007, NeilBrown wrote:
>> [...]
>> > dev_dbg(&dev->sbd.core,
>> > "%s:%u: bio %u: %u segs %u sectors from %lu\n",
>> > - __func__, __LINE__, i, bio_segments(bio),
>> > - bio_sectors(bio), sector);
>> > - bio_for_each_segment(bvec, bio, j) {
>> > + __func__, __LINE__, i, bio_segments(iter.bio),
>> > + bio_sectors(iter.bio),
>> > + (unsigned long)iter.bio->bi_sector);
>> ^^^^^^^^^^^^^^^
>> Superfluous cast: PS3 is 64-bit only, and casts are evil.

bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
so since sector_t may just change its shape underneath. It would not be so much
of a problem if printf() was not a varargs function, but it is, and hence,
passing an object bigger than the format specifier can make problems.

Jan
--

2007-08-18 14:36:18

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

On Sat, 18 Aug 2007, Jan Engelhardt wrote:

> On Aug 18 2007 20:07, Satyam Sharma wrote:
> >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> >
> >> On Thu, 16 Aug 2007, NeilBrown wrote:
> >> [...]
> >> > dev_dbg(&dev->sbd.core,
> >> > "%s:%u: bio %u: %u segs %u sectors from %lu\n",
> >> > - __func__, __LINE__, i, bio_segments(bio),
> >> > - bio_sectors(bio), sector);
> >> > - bio_for_each_segment(bvec, bio, j) {
> >> > + __func__, __LINE__, i, bio_segments(iter.bio),
> >> > + bio_sectors(iter.bio),
> >> > + (unsigned long)iter.bio->bi_sector);
> >> ^^^^^^^^^^^^^^^
> >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
>
> bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> so since sector_t may just change its shape underneath. It would not be so much
> of a problem if printf() was not a varargs function, but it is, and hence,
> passing an object bigger than the format specifier can make problems.

Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
I was mentioning why the cast itself should be (unsigned long long) otoh.

2007-08-20 11:21:25

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

On Sat, 18 Aug 2007, Satyam Sharma wrote:
> On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > >> [...]
> > >> > dev_dbg(&dev->sbd.core,
> > >> > "%s:%u: bio %u: %u segs %u sectors from %lu\n",
> > >> > - __func__, __LINE__, i, bio_segments(bio),
> > >> > - bio_sectors(bio), sector);
> > >> > - bio_for_each_segment(bvec, bio, j) {
> > >> > + __func__, __LINE__, i, bio_segments(iter.bio),
> > >> > + bio_sectors(iter.bio),
> > >> > + (unsigned long)iter.bio->bi_sector);
> > >> ^^^^^^^^^^^^^^^
> > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> >
> > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > so since sector_t may just change its shape underneath. It would not be so much
> > of a problem if printf() was not a varargs function, but it is, and hence,
> > passing an object bigger than the format specifier can make problems.
>
> Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> I was mentioning why the cast itself should be (unsigned long long) otoh.

On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
no compiler warning to be silenced by adding the cast.

On the other hand, adding a cast may hide bugs:
- cast to unsigned long: When reusing parts of this code for a new 32-bit
driver, the sector_t will be silently truncated,
- cast to unsigned long long: When sector_t is changed to an even larger
type, it will be silently truncated as well.
Without the cast, these would cause compiler warnings.

BTW, the resulting code didn't even compile, because of 3 bugs:

| linux-2.6/drivers/block/ps3disk.c: In function ‘ps3disk_scatter_gather’:
| linux-2.6/drivers/block/ps3disk.c:115: error: ‘bio’ undeclared (first use in this function)
| linux-2.6/drivers/block/ps3disk.c:115: error: (Each undeclared identifier is reported only once
| linux-2.6/drivers/block/ps3disk.c:115: error: for each function it appears in.)
| linux-2.6/drivers/block/ps3disk.c:115: error: ‘j’ undeclared (first use in this function)
| linux-2.6/drivers/block/ps3disk.c:116: error: implicit declaration of function ‘bio_kunmap_bvec’
| make[3]: *** [drivers/block/ps3disk.o] Error 1

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

2007-08-20 12:46:45

by Jan Engelhardt

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

On Aug 20 2007 13:21, Geert Uytterhoeven wrote:
>> > >> > dev_dbg(&dev->sbd.core,
>> > >> > "%s:%u: bio %u: %u segs %u sectors from %lu\n",
>> > >> > - __func__, __LINE__, i, bio_segments(bio),
>> > >> > - bio_sectors(bio), sector);
>> > >> > - bio_for_each_segment(bvec, bio, j) {
>> > >> > + __func__, __LINE__, i, bio_segments(iter.bio),
>> > >> > + bio_sectors(iter.bio),
>> > >> > + (unsigned long)iter.bio->bi_sector);
>> > >> ^^^^^^^^^^^^^^^
>> > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
>> >
>> > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
>> > so since sector_t may just change its shape underneath. It would not be so much
>> > of a problem if printf() was not a varargs function, but it is, and hence,
>> > passing an object bigger than the format specifier can make problems.
>>
>> Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
>> I was mentioning why the cast itself should be (unsigned long long) otoh.
>
>On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
>on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
>no compiler warning to be silenced by adding the cast.

Note actually.. I was accidentally referring to printf rather than the
actual ^^^-marked cast. Sorry for the inconvenience.

Jan
--

2007-08-21 01:25:57

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

Hi Geert,

On Mon, 20 Aug 2007, Geert Uytterhoeven wrote:

> On Sat, 18 Aug 2007, Satyam Sharma wrote:
> > On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > > >> [...]
> > > >> > dev_dbg(&dev->sbd.core,
> > > >> > "%s:%u: bio %u: %u segs %u sectors from %lu\n",
^^^

> > > >> > - __func__, __LINE__, i, bio_segments(bio),
> > > >> > - bio_sectors(bio), sector);
> > > >> > - bio_for_each_segment(bvec, bio, j) {
> > > >> > + __func__, __LINE__, i, bio_segments(iter.bio),
> > > >> > + bio_sectors(iter.bio),
> > > >> > + (unsigned long)iter.bio->bi_sector);
^^^^^^^^^^^^^^^ ^^^^^^^^^

> > > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> > >
> > > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > > so since sector_t may just change its shape underneath.
> >
> > Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> > I was mentioning why the cast itself should be (unsigned long long) otoh.
>
> On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
> on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
> no compiler warning to be silenced by adding the cast.

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 $B!F(J%lu$B!G(J expects type $B!F(Jlong unsigned int$B!G(J, but argument 2 has
type $B!F(Jsector_t$B!G(J

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. ]

* 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 $B!F(J%llu$B!G(J expects type $B!F(Jlong long unsigned int$B!G(J, but argument
2 has type $B!F(Jsector_t$B!G(J

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):

Signed-off-by: Satyam Sharma <[email protected]>

---

diff -ruNp a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
--- a/drivers/block/ps3disk.c 2007-08-21 06:52:34.000000000 +0530
+++ b/drivers/block/ps3disk.c 2007-08-21 06:56:07.000000000 +0530
@@ -100,10 +100,10 @@ static void ps3disk_scatter_gather(struc
rq_for_each_segment(bvec, req, iter) {
unsigned long flags;
dev_dbg(&dev->sbd.core,
- "%s:%u: bio %u: %u segs %u sectors from %lu\n",
+ "%s:%u: bio %u: %u segs %u sectors from %llu\n",
__func__, __LINE__, i, bio_segments(iter.bio),
bio_sectors(iter.bio),
- (unsigned long)iter.bio->bi_sector);
+ (unsigned long long)iter.bio->bi_sector);

size = bvec->bv_len;
buf = bvec_kmap_irq(bvec, flags);
@@ -142,8 +142,9 @@ static int ps3disk_submit_request_sg(str

start_sector = req->sector * priv->blocking_factor;
sectors = req->nr_sectors * priv->blocking_factor;
- dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
- __func__, __LINE__, op, sectors, start_sector);
+ dev_dbg(&dev->sbd.core, "%s:%u: %s %llu sectors starting at %llu\n",
+ __func__, __LINE__, op, (unsigned long long)sectors,
+ (unsigned long long)start_sector);

if (write) {
ps3disk_scatter_gather(dev, req, 1);

2007-08-21 07:09:45

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

On Tue, 21 Aug 2007, Satyam Sharma wrote:
> On Mon, 20 Aug 2007, Geert Uytterhoeven wrote:
> > On Sat, 18 Aug 2007, Satyam Sharma wrote:
> > > On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > > > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > > > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > > > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > > > >> [...]
> > > > >> > dev_dbg(&dev->sbd.core,
> > > > >> > "%s:%u: bio %u: %u segs %u sectors from %lu\n",
> ^^^
>
> > > > >> > - __func__, __LINE__, i, bio_segments(bio),
> > > > >> > - bio_sectors(bio), sector);
> > > > >> > - bio_for_each_segment(bvec, bio, j) {
> > > > >> > + __func__, __LINE__, i, bio_segments(iter.bio),
> > > > >> > + bio_sectors(iter.bio),
> > > > >> > + (unsigned long)iter.bio->bi_sector);
> ^^^^^^^^^^^^^^^ ^^^^^^^^^
>
> > > > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> > > >
> > > > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > > > so since sector_t may just change its shape underneath.
> > >
> > > Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> > > I was mentioning why the cast itself should be (unsigned long long) otoh.
> >
> > On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
> > on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
> > no compiler warning to be silenced by adding the cast.
>
> 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 ??long unsigned int??, but argument 2 has
> type ??sector_t??

You will get a warning, so you will know.

> 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 ]

> * 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 ??long long unsigned int??, but argument
> 2 has type ??sector_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.

> Signed-off-by: Satyam Sharma <[email protected]>

NAK. Do not add casts that are not needed. Casts are evil.

> diff -ruNp a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
> --- a/drivers/block/ps3disk.c 2007-08-21 06:52:34.000000000 +0530
> +++ b/drivers/block/ps3disk.c 2007-08-21 06:56:07.000000000 +0530
> @@ -100,10 +100,10 @@ static void ps3disk_scatter_gather(struc
> rq_for_each_segment(bvec, req, iter) {
> unsigned long flags;
> dev_dbg(&dev->sbd.core,
> - "%s:%u: bio %u: %u segs %u sectors from %lu\n",
> + "%s:%u: bio %u: %u segs %u sectors from %llu\n",
> __func__, __LINE__, i, bio_segments(iter.bio),
> bio_sectors(iter.bio),
> - (unsigned long)iter.bio->bi_sector);
> + (unsigned long long)iter.bio->bi_sector);
>
> size = bvec->bv_len;
> buf = bvec_kmap_irq(bvec, flags);
> @@ -142,8 +142,9 @@ static int ps3disk_submit_request_sg(str
>
> start_sector = req->sector * priv->blocking_factor;
> sectors = req->nr_sectors * priv->blocking_factor;
> - dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
> - __func__, __LINE__, op, sectors, start_sector);
> + dev_dbg(&dev->sbd.core, "%s:%u: %s %llu sectors starting at %llu\n",
> + __func__, __LINE__, op, (unsigned long long)sectors,
> + (unsigned long long)start_sector);
>
> if (write) {
> ps3disk_scatter_gather(dev, req, 1);

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

2007-08-21 09:35:35

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

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

2007-08-21 09:52:53

[permalink] [raw]

Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

On Tue, 21 Aug 2007, Satyam Sharma wrote:
> 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?

If it's code for a PS3-specific driver that cannot work in 32-bit mode, yes.

> > > 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.

IC.

> > > * 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

Yes. Yes.

> 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).

Not silent truncation. Without a cast, the compiler will warn.

> 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.

OK, thx.
The driver uses the PS3-specific hypervisor interface, which is 64-bit.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

2007-08-21 10:28:52

[permalink] [raw]

Subject: [PATCH] PS3: Update MAINTAINERS

On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:

> On Tue, 21 Aug 2007, Satyam Sharma wrote:
> > On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
> > >
> > > [ 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.
>
> IC.

[PATCH] PS3: Update MAINTAINERS

Cell Broadband Engine OSS Development <[email protected]> is an
subscribers-only mailing list.

Signed-off-by: Satyam Sharma <[email protected]>

---

MAINTAINERS | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 371fe67..ec1bc77 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3029,14 +3029,14 @@ PS3 NETWORK SUPPORT
P: Masakazu Mokuno
M: [email protected]
L: [email protected]
-L: [email protected]
+L: [email protected] (subscribers-only)
S: Supported

PS3 PLATFORM SUPPORT
P: Geoff Levand
M: [email protected]
L: [email protected]
-L: [email protected]
+L: [email protected] (subscribers-only)
S: Supported

PVRUSB2 VIDEO4LINUX DRIVER

2007-08-21 19:01:28

by Geoff Levand

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

Satyam Sharma wrote:
>
> On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
>
>> On Tue, 21 Aug 2007, Satyam Sharma wrote:
>> > On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
>> > >
>> > > [ 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.
>>
>> IC.
>
> [PATCH] PS3: Update MAINTAINERS
>
> Cell Broadband Engine OSS Development <[email protected]> is an
> subscribers-only mailing list.
>
> Signed-off-by: Satyam Sharma <[email protected]>
>
> ---
>
> MAINTAINERS | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 371fe67..ec1bc77 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3029,14 +3029,14 @@ PS3 NETWORK SUPPORT
> P: Masakazu Mokuno
> M: [email protected]
> L: [email protected]
> -L: [email protected]
> +L: [email protected] (subscribers-only)
> S: Supported
>
> PS3 PLATFORM SUPPORT
> P: Geoff Levand
> M: [email protected]
> L: [email protected]
> -L: [email protected]
> +L: [email protected] (subscribers-only)
> S: Supported

Hi Hugh,

Could you verify that cbe-oss-dev is a subscriber's
only list? I didn't see anything that said it
specifically in the listinfo.

-Geoff

2007-08-21 20:01:54

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

On Tue, 2007-08-21 at 12:01 -0700, Geoff Levand wrote:
> Could you verify that cbe-oss-dev is a subscriber's
> only list? I didn't see anything that said it
> specifically in the listinfo.

This is the "hold" message I got on posting to that list.

Your mail to 'cbe-oss-dev' with the subject

Re: [PATCH] [392/2many] MAINTAINERS - PS3 PLATFORM SUPPORT

Is being held until the list moderator can review it for approval.

The reason it is being held:

Post by non-member to a members-only list

2007-08-21 20:10:49

by Geoff Levand

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

Satyam Sharma wrote:
>
> On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
>
>> On Tue, 21 Aug 2007, Satyam Sharma wrote:
>> > On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
>> > >
>> > > [ 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.
>>
>> IC.
>
> [PATCH] PS3: Update MAINTAINERS
>
> Cell Broadband Engine OSS Development <[email protected]> is an
> subscribers-only mailing list.
>
> Signed-off-by: Satyam Sharma <[email protected]>
>
> ---
>
> MAINTAINERS | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 371fe67..ec1bc77 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3029,14 +3029,14 @@ PS3 NETWORK SUPPORT
> P: Masakazu Mokuno
> M: [email protected]
> L: [email protected]
> -L: [email protected]
> +L: [email protected] (subscribers-only)
> S: Supported
>
> PS3 PLATFORM SUPPORT
> P: Geoff Levand
> M: [email protected]
> L: [email protected]
> -L: [email protected]
> +L: [email protected] (subscribers-only)
> S: Supported

Sorry for any inconvenience this may have caused to any
non-subscribers who posted to that list.

Acked-by: Geoff Levand <[email protected]>

2007-08-21 20:11:56

by Rene Herman

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

On 08/21/2007 10:01 PM, Joe Perches wrote:

> On Tue, 2007-08-21 at 12:01 -0700, Geoff Levand wrote:

>> Could you verify that cbe-oss-dev is a subscriber's
>> only list? I didn't see anything that said it
>> specifically in the listinfo.
>
> This is the "hold" message I got on posting to that list.
>
> Your mail to 'cbe-oss-dev' with the subject
>
> Re: [PATCH] [392/2many] MAINTAINERS - PS3 PLATFORM SUPPORT
>
> Is being held until the list moderator can review it for approval.
>
> The reason it is being held:
>
> Post by non-member to a members-only list

That means it's not a subcriber-only list -- the message wasn't rejected,
just subjected to moderation.

Rene.

2007-08-21 20:17:52

by Geoff Levand

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

Rene Herman wrote:
> On 08/21/2007 10:01 PM, Joe Perches wrote:
>
>> On Tue, 2007-08-21 at 12:01 -0700, Geoff Levand wrote:
>
>>> Could you verify that cbe-oss-dev is a subscriber's
>>> only list? I didn't see anything that said it
>>> specifically in the listinfo.
>>
>> This is the "hold" message I got on posting to that list.
>>
>> Your mail to 'cbe-oss-dev' with the subject
>>
>> Re: [PATCH] [392/2many] MAINTAINERS - PS3 PLATFORM SUPPORT
>>
>> Is being held until the list moderator can review it for approval.
>>
>> The reason it is being held:
>>
>> Post by non-member to a members-only list
>
> That means it's not a subcriber-only list -- the message wasn't rejected,
> just subjected to moderation.
>

So maybe it would be more precise to have something like this:

-L: [email protected]
+L: [email protected] (moderated)

2007-08-21 20:36:29

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

Hi,

On Tue, 21 Aug 2007, Geoff Levand wrote:

> Rene Herman wrote:
> > On 08/21/2007 10:01 PM, Joe Perches wrote:
> >
> >> On Tue, 2007-08-21 at 12:01 -0700, Geoff Levand wrote:
> >
> >>> Could you verify that cbe-oss-dev is a subscriber's
> >>> only list? I didn't see anything that said it
> >>> specifically in the listinfo.
> >>
> >> This is the "hold" message I got on posting to that list.
> >>
> >> Your mail to 'cbe-oss-dev' with the subject
> >>
> >> Re: [PATCH] [392/2many] MAINTAINERS - PS3 PLATFORM SUPPORT
> >>
> >> Is being held until the list moderator can review it for approval.
> >>
> >> The reason it is being held:
> >>
> >> Post by non-member to a members-only list
> >
> > That means it's not a subcriber-only list -- the message wasn't rejected,
> > just subjected to moderation.

That's precisely a subscribers-only list. At least as per MAINTAINER's
definition of that term. It'd be surprising to know of a mailing list
mentioned in MAINTAINERS that _drops_ messages posted to it entirely!

> So maybe it would be more precise to have something like this:
>
> -L: [email protected]
> +L: [email protected] (moderated)

Exactly a similar message greets those who post to linux-arm-kernel@
and it (and other such lists) is referred to everywhere in that file
as (subscribers-only), for example. The patch is correct, IMO.

Thanks,

Satyam

2007-08-21 20:38:23

by Rene Herman

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

On 08/21/2007 10:17 PM, Geoff Levand wrote:

> Rene Herman wrote:
>> On 08/21/2007 10:01 PM, Joe Perches wrote:

>>> The reason it is being held:
>>>
>>> Post by non-member to a members-only list
>> That means it's not a subcriber-only list -- the message wasn't rejected,
>> just subjected to moderation.
>>
>
> So maybe it would be more precise to have something like this:
>
> -L: [email protected]
> +L: [email protected] (moderated)

Perhaps. In these spamridden days, mailinglists that don't have the kind of
(human and other) resources behind them that linux-kernel has basically have
the choice between drowning in spam, becoming subscriber only or moderate
non-subscribers and of these, that third option is "best among the bad".

alsa-devel for example also went this route -- the spam levels simply
weren't tolerable anymore for any subscriber and the list was dying as a
result. Moderation sucks, but subcriber-only sucks even worse (generally,
and/but even more so for lists that expect crossposts from linux-kernel) so
what's a small-time list to do.

Moderation takes some effort so the lists that moderate have made the
explicit choice to not become subscriber-only. While subscriber-only
certainly is useful to mention alongside the list address itself, I'm not
too sure mentioning moderation makes a great deal of sense actually -- if
all's well, the moderator will simply approve it and you don't have to deal
with it other than that.

Rene.

2007-08-21 20:46:19

by Rene Herman

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

On 08/21/2007 10:48 PM, Satyam Sharma wrote:

>>> That means it's not a subcriber-only list -- the message wasn't rejected,
>>> just subjected to moderation.
>
> That's precisely a subscribers-only list. At least as per MAINTAINER's
> definition of that term. It'd be surprising to know of a mailing list
> mentioned in MAINTAINERS that _drops_ messages posted to it entirely!

Then its definition does not make any sense whatsoever -- as said just now
as well, moderation takes some effort so lists that do have made the
explicit choice to _not_ become subscriber-only.

Moderation is furthermore not something the poster needs to concern himself
with, other than through being aware of a possible delay. It's mostly those
poor moderators that wade through bucket-loads of spam that care... ;-)

Rene.

2007-08-21 20:47:38

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

On Wed, 2007-08-22 at 02:18 +0530, Satyam Sharma wrote:
> That's precisely a subscribers-only list. At least as per MAINTAINER's
> definition of that term. It'd be surprising to know of a mailing list
> mentioned in MAINTAINERS that _drops_ messages posted to it entirely!

Here's one possibility, though I did sent messages that
some might have considered spam:

[email protected]

This one isn't marked as subscriber only either.

On Mon, 2007-08-13 at 14:00 -0400, [email protected]
wrote:
> Vous n'êtes pas autorisé à envoyer des messages sur cette liste, votre
> message a été automatiquement rejeté. Si vous pensez que votre message
> a été rejeté par erreur, veuillez contacter le gestionnaire de la
> liste à l'adresse [email protected].

2007-08-21 21:58:24

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

Hi Geoff,

> Could you verify that cbe-oss-dev is a subscriber's
> only list? I didn't see anything that said it
> specifically in the listinfo.

Yes, the list is configured as subscriber only, I make sure I add people to the accept filter as soon as they hit the list.

Fell behind a bit yesterday so apologies for the inconvenience this caused you Satyam.

Rationale is it keeps the SPAM down quite a bit but I acknowledge it has it drawbacks if I don't get to the held messages quickly.

Cheers,
Hugh

2007-08-23 18:13:34

by Matt Mackall

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

On Wed, Aug 22, 2007 at 07:58:13AM +1000, Hugh Blemings wrote:
> Hi Geoff,
>
> > Could you verify that cbe-oss-dev is a subscriber's
> > only list? I didn't see anything that said it
> > specifically in the listinfo.
>
> Yes, the list is configured as subscriber only, I make sure I add people to the accept filter as soon as they hit the list.
>
> Fell behind a bit yesterday so apologies for the inconvenience this caused you Satyam.
>
> Rationale is it keeps the SPAM down quite a bit but I acknowledge it has it drawbacks if I don't get to the held messages quickly.

The drawback is mostly in getting the rejection/moderation message.

Whitelisting is fine. If you configure the list to not send the
moderation warning to legitimate non-subscribers, you'll eliminate 99%
of the annoyance.

--
Mathematics is the supreme nostalgia of our time.

2007-08-24 05:52:42

[permalink] [raw]

Subject: Re: [PATCH] PS3: Update MAINTAINERS

Hi Matt, All,

> The drawback is mostly in getting the rejection/moderation message.
>
> Whitelisting is fine. If you configure the list to not send the
> moderation warning to legitimate non-subscribers, you'll eliminate 99%
> of the annoyance.

Good call, have changed the configuration accordingly.

Thanks for the feedback all :)

Cheers,
Hugh

2007-08-24 06:25:24

[permalink] [raw]

Subject: Re: [Cbe-oss-dev] [PATCH] PS3: Update MAINTAINERS

Hi Satyam, All,

> [PATCH] PS3: Update MAINTAINERS
>
> Cell Broadband Engine OSS Development <[email protected]> is an
> subscribers-only mailing list.
>
> Signed-off-by: Satyam Sharma <[email protected]>
>
> [...]

As per recent discussions...

The list is moderated for SPAM prevention, messages from valid senders
will be forwarded manually and their email added to the whitelist.

I've changed the list config so that it no longer generates the
"subscriber-only" notification.

On this basis I recommended the patch be NAKd so that people aren't put
off copying the list where appropriate.

Thanks,

Regards,
Hugh

2007-08-24 06:35:22

[permalink] [raw]

Subject: Re: [Cbe-oss-dev] [PATCH] PS3: Update MAINTAINERS

On Fri, 24 Aug 2007, Hugh Blemings wrote:

> Hi Satyam, All,
>
> > [PATCH] PS3: Update MAINTAINERS
> >
> > Cell Broadband Engine OSS Development <[email protected]> is an
> > subscribers-only mailing list.
> >
> > Signed-off-by: Satyam Sharma <[email protected]>
> >
> > [...]
>
> As per recent discussions...
>
> The list is moderated for SPAM prevention, messages from valid senders
> will be forwarded manually and their email added to the whitelist.
>
> I've changed the list config so that it no longer generates the
> "subscriber-only" notification.

So ... if someone (!subscriber && !whitelisted) sends a message, it stays
put somewhere till you get to it and determine whether it is relevant or
not, and either (1) forward it on to the list if appropriate, or (2) drop
it if detected as spam. And no "notifications" are ever sent out to
genuine / well-meaning posters during this process. Right?

> On this basis I recommended the patch be NAKd so that people aren't put
> off copying the list where appropriate.

Fair enough, if my understanding (as mentioned above) is correct, then I
believe the patch achieved a greater purpose than what it was originally
sent out for!

Thanks,

Satyam

2007-08-24 06:48:58