AW: fix 2.4.33.3 / sun partition size

AW: fix 2.4.33.3 / sun partition size

[Jurzitza, Dieter]

Dear Willy,
one final thougth here: ist there any real reason to *not* make
add_gd_partition (struct gendisk, int minor, unsigned long start, unsigned long size){...}
(see fs/partitions/check.c)
as:

- within add_gd_partition the two values start and size are assigned to a field of type unsigned long,
- we both agree that there is no reason to use signed ints in this case,
- any byte size conversion issue would be covered by the fact that it does not hurt assigning too small numbers to oversized ones.

Just my two cents here ...

Take care




Dieter


-- 
________________________________________________

HARMAN BECKER AUTOMOTIVE SYSTEMS

Dr.-Ing. Dieter Jurzitza
Manager Hardware Systems
   System Development

Industriegebiet Ittersbach
Becker-Göring Str. 16
D-76307 Karlsbad / Germany

Phone: +49 (0)7248 71-1577
Fax:   +49 (0)7248 71-1216
eMail: DJurzitza@harmanbecker.com
Internet: http://www.becker.de
 


*******************************************
Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtuemlich erhalten haben, informieren Sie bitte sofort den Absender und loeschen Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail ist nicht gestattet.
 
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and delete this e-mail. Any unauthorized copying, disclosure or distribution of the contents in this e-mail is strictly forbidden.
*******************************************

Re: fix 2.4.33.3 / sun partition size

[Willy Tarreau]

[Andries CCed since he's the partition maintainer]

Hi Dieter,

On Wed, Sep 13, 2006 at 07:40:26AM +0200, Jurzitza, Dieter wrote:
> Dear Willy,
> one final thougth here: ist there any real reason to *not* make
> add_gd_partition (struct gendisk, int minor, unsigned long start, unsigned long size){...}
> (see fs/partitions/check.c)
> as:
> 
> - within add_gd_partition the two values start and size are assigned to a field of type unsigned long,
> - we both agree that there is no reason to use signed ints in this case,
> - any byte size conversion issue would be covered by the fact that it does not hurt assigning too small numbers to oversized ones.
> 
> Just my two cents here ...

I agree with your analysis here. I've checked the code right now, and using signed
ints in add_gd_partition() makes no sense to me as the hd_struct uses unsigned longs.
So a cast is performed within add_gd_partition() anyway.

We should make add_gd_partition() accept unsigned longs so that only its users will
have to cast to unsigned long if needed, and this way it will be easier to track
invalid signedness use.

I've quickly checked if 2.6 needs a fix, but 2.6 declares add_partition() with
sector_t values which are unsigned longs. So 2.6 is safe right now. I will add
the fix and check at least on x86 and sparc64 that I do not see any regression
nor warnings (I don't have 1TB to check larger partitions!).

Thanks,
Willy

fix 2.4.33.3 / sun partition size

[Jurzitza, Dieter]

Kernel: 2.4.33

Issue: really fix size display for sun partitions larger than 1TByte

Signed off by: Dieter Jurzitza DJurzitza@HarmanBecker.com

Problem: the last fix introduced by Jeff Mahoney for kernel 2.6 was not complete for kernel 2.4 (as applied)
I found out that add_gd_partition is called by any type of partition (2.4). add_gd_partition is defined as add_gd_partition (int, int), what makes no sense to me as negative numbers should never occur here. As long as add_gd_partition is not changed to add_gd_partition (unsigned, unsigned), /proc/partitions will keep showing negative numbers.

If ever someone could look into this, within the different partition type files in linux/fs/partitions the parameters to add_gd_partitions seem to be chosen arbitrarily between int, unsigned and unsigned long, whatever seemed to be appropriate, I think it would make sense to get consistent parameters to add_gd_partition from all partition types here.
Especially if one takes into account that sizeof (long) and sizeof (int) may differ significantly i. e. on sparc.


Take care


Dieter Jurzitza



--- linux/fs/partitions/sun.c   2002-11-29 00:53:15.000000000 +0100
+++ linux/fs/partitions/sun.c   2006-08-29 07:04:56.000000000 +0200
@@ -86,7 +86,7 @@
        spc = be16_to_cpu(label->ntrks) * be16_to_cpu(label->nsect);
        for (i = 0; i < 8; i++, p++) {
                unsigned long st_sector;
-               int num_sectors;
+               unsigned int num_sectors;
 
                st_sector = first_sector + be32_to_cpu(p->start_cylinder) * spc;
                num_sectors = be32_to_cpu(p->num_sectors);
--- linux/fs/partitions/check.c 2006-08-31 19:03:20.000000000 +0200
+++ linux/fs/partitions/check.c 2006-09-03 12:47:55.000000000 +0200
@@ -204,7 +204,7 @@
 /*
  * Add a partitions details to the devices partition description.
  */
-void add_gd_partition(struct gendisk *hd, int minor, int start, int size)
+void add_gd_partition(struct gendisk *hd, int minor, unsigned int start, unsign
ed int size)
 {
 #ifndef CONFIG_DEVFS_FS
        char buf[40];
--- linux/fs/partitions/check.h 2006-08-31 19:03:20.000000000 +0200
+++ linux/fs/partitions/check.h 2006-09-03 12:48:54.000000000 +0200
@@ -2,7 +2,7 @@
  * add_partition adds a partitions details to the devices partition
  * description.
  */
-void add_gd_partition(struct gendisk *hd, int minor, int start, int size);
+void add_gd_partition(struct gendisk *hd, int minor, unsigned int start, unsign
ed int size);
 
 typedef struct {struct page *v;} Sector;
 


-- 
________________________________________________

HARMAN BECKER AUTOMOTIVE SYSTEMS

Dr.-Ing. Dieter Jurzitza
Manager Hardware Systems
   System Development

Industriegebiet Ittersbach
Becker-Göring Str. 16
D-76307 Karlsbad / Germany

Phone: +49 (0)7248 71-1577
Fax:   +49 (0)7248 71-1216
eMail: DJurzitza@harmanbecker.com
Internet: http://www.becker.de
 


*******************************************
Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtuemlich erhalten haben, informieren Sie bitte sofort den Absender und loeschen Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail ist nicht gestattet.
 
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and delete this e-mail. Any unauthorized copying, disclosure or distribution of the contents in this e-mail is strictly forbidden.
*******************************************

Re: fix 2.4.33.3 / sun partition size

[Willy Tarreau]

On Tue, Sep 12, 2006 at 01:23:56PM +0200, Jurzitza, Dieter wrote:
> Kernel: 2.4.33
> 
> Issue: really fix size display for sun partitions larger than 1TByte
> 
> Signed off by: Dieter Jurzitza DJurzitza@HarmanBecker.com
> 
> Problem: the last fix introduced by Jeff Mahoney for kernel 2.6 was not complete for kernel 2.4 (as applied)
> I found out that add_gd_partition is called by any type of partition (2.4). add_gd_partition is defined as add_gd_partition (int, int), what makes no sense to me as negative numbers should never occur here. As long as add_gd_partition is not changed to add_gd_partition (unsigned, unsigned), /proc/partitions will keep showing negative numbers.

It seems fair. David, what's your opinion ?

> If ever someone could look into this, within the different partition type files in linux/fs/partitions the parameters to add_gd_partitions seem to be chosen arbitrarily between int, unsigned and unsigned long, whatever seemed to be appropriate, I think it would make sense to get consistent parameters to add_gd_partition from all partition types here.
> Especially if one takes into account that sizeof (long) and sizeof (int) may differ significantly i. e. on sparc.

It would really depend on the on-disk format. If the partition table really
stores 32 bit ints for sector counts, there's no point switching from ints
to longs. But if it already stores 64 bits, then we're limiting it to 2 TB
with 32 bit ints. I haven't checked the code right now, so I don't know. I
hope Davem will enlighten us on this matter.

> Take care
> 
> 
> Dieter Jurzitza

Thanks,
Willy

Re: fix 2.4.33.3 / sun partition size

[Tom 'spot' Callaway]

On Tue, 2006-09-12 at 21:17 +0200, Willy Tarreau wrote:
> On Tue, Sep 12, 2006 at 01:23:56PM +0200, Jurzitza, Dieter wrote:
> > Kernel: 2.4.33
> > 
> > Issue: really fix size display for sun partitions larger than 1TByte
> > 
> > Signed off by: Dieter Jurzitza DJurzitza@HarmanBecker.com
> > 
> > Problem: the last fix introduced by Jeff Mahoney for kernel 2.6 was not complete for kernel 2.4 (as applied)
> > I found out that add_gd_partition is called by any type of partition (2.4). add_gd_partition is defined as add_gd_partition (int, int), what makes no sense to me as negative numbers should never occur here. As long as add_gd_partition is not changed to add_gd_partition (unsigned, unsigned), /proc/partitions will keep showing negative numbers.
> 
> It seems fair. David, what's your opinion ?
> 
> > If ever someone could look into this, within the different partition type files in linux/fs/partitions the parameters to add_gd_partitions seem to be chosen arbitrarily between int, unsigned and unsigned long, whatever seemed to be appropriate, I think it would make sense to get consistent parameters to add_gd_partition from all partition types here.
> > Especially if one takes into account that sizeof (long) and sizeof (int) may differ significantly i. e. on sparc.
> 
> It would really depend on the on-disk format. If the partition table really
> stores 32 bit ints for sector counts, there's no point switching from ints
> to longs. But if it already stores 64 bits, then we're limiting it to 2 TB
> with 32 bit ints. I haven't checked the code right now, so I don't know. I
> hope Davem will enlighten us on this matter.

I think Davem may be on vacation...

~spot
-- 
Tom "spot" Callaway || Red Hat || Fedora || Aurora || GPG ID: 93054260

"We must not confuse dissent with disloyalty. We must remember always
that accusation is not proof and that conviction depends upon evidence
and due process of law. We will not walk in fear, one of another. We
will not be driven by fear into an age of unreason, if we dig deep in
our history and our doctrine, and remember that we are not descended
from fearful men -- not from men who feared to write, to speak, to
associate and to defend causes that were, for the moment, unpopular."
-- Edward R. Murrow, March 9, 1954