Re: Commit: sector size fixes

[Patrick Leslie Polzer]

Hello Guillaume,

first, thanks for the quick review.
  It is really appreciated, as the matter is a bit complicated and
changes are all over various parts of libparted.
  Especially GPT needs testing, as there is a lot of heap work going on
there, whereas before all happened on the stack with fixed sizes.


On Thu, 10 Nov 2005 15:15:31 +0100
K.G. <"K.G." <address@hidden>> wrote:

> On Thu, 10 Nov 2005 13:57:54 +0100, Patrick Leslie Polzer
> <address@hidden> wrote:
> 
>       dev->bios_geom.cylinders
> -             = dev->length / (63 * 255)
> -                     / (dev->sector_size /
> PED_SECTOR_SIZE);
> +             = dev->length / (63 * 255) / dev->sector_size;
> I'm not sure about that...
This is wrong, the second one is right.
Reasoning: is holds that

  dev->length = C * H * S

thus

      dev->length
  C = -----------
         H * S

where dev->length is in sectors and C * H * S also because C * H are
unit-neutral and S is sectors. Do you agree?

> and not about that too :
> -                             / (dev->sector_size /
> PED_SECTOR_SIZE);
> +                          / 1;
>  
>               /* what should we stick in here? */
> -             dev->length = dev_stat.st_size / PED_SECTOR_SIZE;
> +             dev->length = dev_stat.st_size / dev->sector_size;
>               dev->bios_geom.cylinders = dev->length / 4 / 32;
>               dev->bios_geom.heads = 4;
>               dev->bios_geom.sectors = 32;
> -             dev->sector_size = PED_SECTOR_SIZE;
> +             dev->sector_size = PED_SECTOR_SIZE_DEFAULT;
> Is dev->sector_size used before initialisation ?
Thanks for spotting that one!

> (or maybe it's initialised before, then changed?)
Not really. We only get to this line if _device_probe_geometry()
fails (which calls the sector_size initializer. So this change is indeed
wrong and we have to use PED_SECTOR_SIZE_DEFAULT. Fixed.
 
> In libparted/disk_gpt.c there are some random changes from tab
> to 8 spaces.  Maybe you could convert the whole file if you really
> like spaces... :)
I do ;) so I will convert it.
I never notice any difference because Vim converts tabs automatically
for me.

> The (PedDevice*) cast in loop_alloc call is uneeded.
Yes. Fixed.

> disk_amiga starts with PED_SECTOR_SIZE --> PED_SECTOR_SIZE_DEFAULT
That's the intention.

> but then :
> -     if ((rdb=RDSK(ped_malloc(PED_SECTOR_SIZE)))==NULL)
> +     if ((rdb=RDSK(ped_malloc(dev->sector_size)))==NULL)
>               return 0;
>       found = _amiga_find_rdb (dev, rdb);
>       ped_free (rdb);
> PED_SECTOR_SIZE --> dev->sector_size. If this is volontary then a
> comment is welcome.
I cannot remember exactly, but I think this *was* my intention
(there were about 8 occurences of it).
I removed it now, though. We have to see whether Amiga PTs support
bs != 512.

> I think the code that is not sector != 512B ready must
> be protected by tests in most important entry points
> (probe, alloc, read, write). For example i'm not sure
> if disk_amiga, disk_mac and disk_dos (and others)
> can really support non 512 bytes sectors.
Right, that went through my mind, too (seemingly after I edited
disk_amiga.c).

> If they don't then using PED_SECTOR_SIZE_DEFAULT and the
> tests would be enough.
> For example in the current state the combination of the
> HFS code with sector != 512 would be extremly dangerous
> and result in overflows.
> I think we should enumerate the formats that are well
> defined enough to support non 512B sectors, and formats
> that are not well defined enough. We should then decide
> weither we should access the device 512 bytes by 512
> bytes or just reject the access.
I like this idea for the start, it can be done easily and
provides a sane state.

> There might be a need for new ped_device_read like APIs
> that takes multiples sizes issues in consideration.
Yes, that is a possible way to go.

> Disclaimer : I didn't read everything :P 
> (this is an enormous commit...)
Hopefully others will help, too.


Kind regards,

Leslie

-- 
PGP-KID: 0x52D70289

00000000.mimetmp
Description: PGP signature

pgpybSDLpYcad.pgp
Description: PGP signature