Re: Commit: sector size fixes

[K.G.]

On Thu, 10 Nov 2005 13:57:54 +0100, Patrick Leslie Polzer <address@hidden> 
wrote:
> On Thu, 10 Nov 2005 13:20:55 +0100
> Patrick Leslie Polzer <Patrick Leslie Polzer <address@hidden>>
> wrote:
> 
> > linux.c now reports real sector size and has a stub for setting the
> > physical sector size.
> > Made disk_gpt.c compatible for sector sizes != 512 (please test!!).
> > Replaced PED_SECTOR_SIZE with PED_SECTOR_SIZE_DEFAULT.
> Also, print now shows the logical and physical sector size (the latter
> currently is always -1, should default to "512" in parted.c before
> release).

Here is a (too) quick review :

In linux.c, near line 455 :
        /* The GETGEO ioctl is no longer useful (as of linux 2.6.x).  We could
         * still use it in 2.4.x, but this is contentious.  Perhaps we should
         * move to EDD. */
        dev->bios_geom.sectors = 63;
        dev->bios_geom.heads = 255;
        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...
and not about that too :
@@ -461,7 +462,7 @@
                dev->hw_geom.cylinders
                        = dev->length / (dev->hw_geom.heads
                                         * dev->hw_geom.sectors)
-                               / (dev->sector_size / PED_SECTOR_SIZE);
+                          / 1;
        } else {
                dev->hw_geom = dev->bios_geom;
        }


@@ -785,11 +788,11 @@
                }
 
                /* 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 ?
(or maybe it's initialised before, then changed?)

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

In libparted/disk_loop.c :
@@ -35,17 +35,17 @@
 
 static PedDiskType loop_disk_type;
 
-static PedDisk* loop_alloc (PedDevice* dev);
+static PedDisk* loop_alloc (const PedDevice* dev);
 static void loop_free (PedDisk* disk);
 
 static int
-loop_probe (PedDevice* dev)
+loop_probe (const PedDevice* dev)
 {
        PedDisk*        disk;
        char            buf [512];
        int             result;
 
-       disk = loop_alloc (dev);
+       disk = loop_alloc ((PedDevice*)dev);
        if (!disk)
                goto error;
The (PedDevice*) cast in loop_alloc call is uneeded.

In disk_amiga :
@@ -150,7 +150,7 @@
        sum = PED_BE32_TO_CPU (rdb[0]);
        end = PED_BE32_TO_CPU (rdb[1]);
 
-       if (end > PED_SECTOR_SIZE) end = PED_SECTOR_SIZE;
+       if (end > PED_SECTOR_SIZE_DEFAULT) end = PED_SECTOR_SIZE_DEFAULT;
 
        for (i = 1; i < end; i++) sum += PED_BE32_TO_CPU (rdb[i]);
disk_amiga starts with PED_SECTOR_SIZE --> PED_SECTOR_SIZE_DEFAULT
but then :
@@ -334,13 +334,13 @@
 static PedDiskType amiga_disk_type;
 
 static int
-amiga_probe (PedDevice *dev)
+amiga_probe (const PedDevice *dev)
 {
        struct RigidDiskBlock *rdb;
        uint32_t found;
        PED_ASSERT(dev != NULL, return 0);
 
-       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 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. 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. There might be a need
for new ped_device_read like APIs that takes multiples
sizes issues in consideration.

Disclaimer : I didn't read everything :P 
(this is an enormous commit...)

Cheers,
Guillaume