[parted-devel] [PATCH] Properly sync partitions with operating system
Joel Granados
jgranado at redhat.com
Fri Feb 20 10:24:10 UTC 2009
On Thu, Feb 19, 2009 at 07:01:33PM +0100, Jim Meyering wrote:
> Joel Granados Moreno wrote:
> > diff --git a/include/parted/disk.h b/include/parted/disk.h
> > index dfcf39e..9e50716 100644
> > --- a/include/parted/disk.h
> > +++ b/include/parted/disk.h
> > @@ -210,6 +210,7 @@ struct _PedDiskOps {
> > /* other */
> > int (*alloc_metadata) (PedDisk* disk);
> > int (*get_max_primary_partition_count) (const PedDisk* disk);
> > + int (*get_max_partition_count) (const PedDisk* disk);
> > };
> >
> > struct _PedDiskType {
> > @@ -322,7 +323,7 @@ extern PedPartition* ped_disk_extended_partition (const PedDisk* disk);
> > /* internal functions */
> > extern PedDisk* _ped_disk_alloc (const PedDevice* dev, const PedDiskType* type);
> > extern void _ped_disk_free (PedDisk* disk);
> > -
> > +extern int ped_disk_get_max_partition_num(const PedDisk* disk);
>
> Thanks for working on this.
>
> If the only values ever returned are non-negative (as it seems they are),
> then it'd be far more readable to make the return type "unsigned int".
> Otherwise, I have to wonder if some of these functions may return a
> negative value, and write code in each caller to handle that.
>
Seems like a good idea. And: No, they are supposed to be possitive
values. Note this will propagate into other elements of the patch as
well.
> > /** @} */
> >
> > diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
> > index 4a8c9e7..6fd0c75 100644
> > --- a/libparted/arch/linux.c
> > +++ b/libparted/arch/linux.c
> > @@ -279,6 +279,9 @@ struct blkdev_ioctl_param {
> > || (M) == SCSI_CDROM_MAJOR \
> > || ((M) >= SCSI_DISK1_MAJOR && (M) <= SCSI_DISK7_MAJOR))
> >
> > +/* Maximum number of partitions supported by linux. */
> > +#define MAX_NUM_PARTS 64
> > +
> > static char* _device_get_part_path (PedDevice* dev, int num);
> > static int _partition_is_mounted_by_path (const char* path);
> >
> > @@ -2261,45 +2264,92 @@ _blkpg_remove_partition (PedDisk* disk, int n)
> > BLKPG_DEL_PARTITION);
> > }
> >
> > +/*
> > + * The number of partitions that a device can have depends on the kernel.
> > + * If we don't find this value in /sys/block/DEV/range, we will use our own
> > + * value.
> > + */
> > +static int
> > +_device_get_partition_range(PedDevice* dev)
> > +{
> > + int range;
> > + char path[128];
> > + FILE* fp;
> > + bool ok;
> > +
> > + if(snprintf(path, sizeof(path), "/sys/block/%s/range",
> > + basename(dev->path)) < 0)
>
> Unless you're sure that dev->path will never be too long, you
> should also ensure that the snprintf return value is < sizeof path.
> Otherwise, fopen will operate on a truncated string which might even
> name an existing file. Sure, it's not likely, but...
>
Ack. Will change. I also see other functions that might be making the
same mistake. Will look into this.
> > + return MAX_NUM_PARTS;
> > +
> > + fp = fopen(path, "r");
> > + if(!fp)
> > + return MAX_NUM_PARTS;
> > +
> > + ok = fscanf(fp, "%d", &range) == 1;
> > + fclose(fp);
> > + return ok ? range : MAX_NUM_PARTS;
> > +}
> > +
> > +/*
> > + * We sync the partition table in two step process:
> > + * 1. We remove all the partitions from the kernel's tables. The partitions
> > + * will not be removed if the ioctl call fails.
> > + * 2. We add all the partitions that we hold in disk.
> > + *
> > + * To achieve this two step process we must calculate the minimum number of
> > + * maximum possible partitions between what linux supports and what the label
> > + * type supports. EX:
> > + *
> > + * number=MIN(max_parts_supported_in_linux,max_parts_supported_in_msdos_tables)
> > + *
> > + * For now we will hard code the maximum number of partitions for linux to
> > + * MAX_NUM_PARTS. Might want to search for a way to include this from somewhere.
>
> Thanks for adding comments.
>
> > static int
> > _disk_sync_part_table (PedDisk* disk)
> > {
> > - int largest_partnum = ped_disk_get_last_partition_num (disk);
> > - if (largest_partnum <= 0)
> > - return 1;
> > + PED_ASSERT(disk != NULL, return 0);
> > + PED_ASSERT(disk->dev != NULL, return 0);
>
>
> It's not at all clear (from reading only these diffs) that you've made
> only minimal changes to this function. The trouble is that you've
> changed indentation and formatting. So please adjust this patch *not*
> to make those formatting changes and you'll see that it's much smaller --
> and more readable.
>
This is my vim configuration saying. "I'm editing a C file, lest use
tabs". I was conscious of the fact that I was doing this and did not
correct it because IMO tabs are prettier in C code. I guess modifying
the diff is not at all difficult, but I still have something in the back
of my mind that says: "we should be coding with tabs for this project".
Additionally. Are there specific code conventions in the parted
project? If so a code clean up would be nice. It would be one of those
"we will do it someday" things, but nice none the less.
> > - int last = 16;
> > - int* rets = ped_malloc(sizeof(int) * last);
> > - int* errnums = ped_malloc(sizeof(int) * last);
> > - int ret = 1;
> > - int i;
> > + /* lpn = largest partition number. */
> > + int lpn = PED_MIN(_device_get_partition_range(disk->dev),
> > + ped_disk_get_max_partition_num(disk));
> >
> > - for (i = 1; i <= last; i++) {
> > - rets[i - 1] = _blkpg_remove_partition (disk, i);
> > - errnums[i - 1] = errno;
> > - }
> > + /* Its not possible to support largest_partnum <= 0. */
> > + if(lpn <= 0)
> > + return 0;
>
> > - for (i = 1; i <= last; i++) {
> > - const PedPartition *part;
> > + int* rets = ped_malloc(sizeof(int) * lpn);
> > + int* errnums = ped_malloc(sizeof(int) * lpn);
> > + int ret = 1;
> > + int i;
> >
> > - part = ped_disk_get_partition (disk, i);
> > - if (part) {
> > - /* busy... so we won't (can't!) disturb ;) Prolly
> > - * doesn't matter anyway, because users shouldn't be
> > - * changing mounted partitions anyway...
> > - */
> > - if (!rets[i - 1] && errnums[i - 1] == EBUSY)
> > - continue;
> > -
> > - /* add the (possibly modified or new) partition */
> > - if (!_blkpg_add_partition (disk, part))
> > - ret = 0;
> > - }
> > - }
> > + for(i = 1; i <= lpn; i++) {
> > + rets[i - 1] = _blkpg_remove_partition (disk, i);
> > + errnums[i - 1] = errno;
> > + }
> > +
> > + for(i = 1; i <= lpn; i++) {
> > + const PedPartition *part;
> > +
> > + part = ped_disk_get_partition (disk, i);
> > + if (part) {
> > + /* busy... so we won't (can't!) disturb ;) Prolly
> > + * doesn't matter anyway, because users shouldn't be
> > + * changing mounted partitions anyway...
> > + */
> > + if (!rets[i - 1] && errnums[i - 1] == EBUSY)
> > + continue;
> > +
> > + /* add the (possibly modified or new) partition */
> > + if (!_blkpg_add_partition (disk, part))
> > + ret = 0;
> > + }
> > + }
> >
> > - free (rets);
> > - free (errnums);
> > - return ret;
> > + free (rets);
> > + free (errnums);
> > + return ret;
> > }
>
> The rest looks ok.
thx for review.
--
Joel Andres Granados
Brno, Czech Republic, Red Hat.
More information about the parted-devel
mailing list