Subject: Re: bug#16231: [PATCH 1/2] Fix loop labels

From: Phillip Susi <[email-masked]>
Date: Mon, 03 Mar 2014 19:30:43 -0500

On 03/03/2014 01:48 PM, Brian C. Lane wrote:
> I am not sure exactly what the problem is here. I think you are
> saying the Disk identification is including a partition? On Fedora
> 20 (parted 3.1 plus a number of my patches) I am seeing this output
> when using scsi_debug (and when using a /dev/mapper device and when
> using a disk image directly):
> 
> [bcl@lister parted (master *)]$ sudo parted -s /dev/sdd p Model:
> Linux scsi_debug (scsi) Disk /dev/sdd: 94.4MB Sector size
> (logical/physical): 512B/512B Partition Table: loop Disk Flags:
> 
> Number  Start  End     Size    File system  Flags 1      0.00B
> 94.4MB  94.4MB  ext2
> 
> This looks fine to me.

Right, it reports a synthetic partition number 1 starting at offset 0.
 GParted asks libparted for the device name of partition number 1, and
it returns /dev/sdd1, rather than /dev/sdd, which causes gparted to
run for instance, mke2fs /dev/sdd1, which of course, fails.

> I still think we should split up tests into their own commits.

I really prefer the cleaner git log keeping them together gives.

>> 
>> diff --git a/NEWS b/NEWS index 935fa33..816fb57 100644 ---
>> a/NEWS +++ b/NEWS @@ -12,6 +12,8 @@ GNU parted NEWS
>> -*- outline -*- boot partition type.
>> 
>> ** Bug Fixes +  Fix several bugs with loop labels ( whole disk
>> filesystems ) + Fix gpt to correctly handle non ASCII charcters
>> in partition names
>> 
>> If a drive was 100 times an even multiple of two, sizes specified
>> as diff --git a/include/parted/device.in.h
>> b/include/parted/device.in.h index 7c06a66..ff23a6c 100644 ---
>> a/include/parted/device.in.h +++ b/include/parted/device.in.h @@
>> -92,6 +92,7 @@ struct _PedDevice { short           host, did;
>> 
>> void*           arch_specific; +        int             loop;
>> /* using "loop" partition table */ };
> 
> When we make changes like these should we be bumping the library 
> version? Or should that only happen at release time? I'm also not
> sure how to actually do that, not being a big fan of autotools.

I believe that since PedDevice is an opaque structure ( libparted
client's aren't supposed to allocate or copy them directly ), and
since the new member is at the end, it doesn't break ABI.  As for how
to bump the ABI, it's a change of the CURRENT variable in
libparted/Makefile.am.

>> @@ -2814,7 +2819,10 @@ _disk_sync_part_table (PedDisk* disk) int
>> i; /* remove old partitions first */ for (i = 1; i <= lpn; i++)
>> { -                PedPartition *part = ped_disk_get_partition
>> (disk, i); +                PedPartition *part; +
>> if (disk->dev->loop) +                        part = 0; +
>> else part = ped_disk_get_partition (disk, i); if (part) { 
>> unsigned long long length; unsigned long long start; @@ -2830,7
>> +2838,10 @@ _disk_sync_part_table (PedDisk* disk) } } for (i = 1;
>> i <= lpn; i++) { -                PedPartition *part =
>> ped_disk_get_partition (disk, i); +                PedPartition
>> *part; +                if (disk->dev->loop) +
>> part = 0; +                else part = ped_disk_get_partition
>> (disk, i); if (part) { unsigned long long length; unsigned long
>> long start;
> 
> We should be skipping these loops instead of letting them run with
> part set to 0.

The first loop we could just skip, but not the second one because we
do want to remove any existing partitions.  Think of starting with an
existing partitioned disk and running mklabel loop on it.  I'll revise
the patch to skip the first loop.

>> diff --git a/libparted/labels/loop.c b/libparted/labels/loop.c 
>> index ea8f007..ad0db99 100644 --- a/libparted/labels/loop.c +++
>> b/libparted/labels/loop.c @@ -80,7 +80,10 @@ loop_alloc (const
>> PedDevice* dev)
>> 
>> if (dev->length < 256) return NULL; -	return _ped_disk_alloc
>> ((PedDevice*)dev, &loop_disk_type); +	PedDisk *disk =
>> _ped_disk_alloc ((PedDevice*)dev, &loop_disk_type); +	if (disk) +
>> disk->disk_specific = (void *)0; +	return disk; }
> 
> This seems like a misuse of the disk_specific pointer.

I figured it would be silly to allocate a structure to just contain a
single boolean value when the pointer itself can be used for that
purpose.  Admittedly it is a bit of a micro optimization but there's a
part of me that just hates to allocate things you don't really need.
I suppose I could add a comment or two explaining the usage.

>> @@ -156,29 +154,17 @@ static int loop_write (const PedDisk*
>> disk) { size_t buflen = disk->dev->sector_size; -	char *buf =
>> ped_malloc (buflen); -	if (buf == NULL) +	char *buf = alloca
>> (buflen); +	disk->dev->loop = 1; +	/* only write label after
>> creating it new */ +	if (disk->disk_specific) return 0; - -	if
>> (ped_disk_get_partition (disk, 1)) { -		if (!ped_device_read
>> (disk->dev, buf, 0, 1)) { -			free (buf); -			return 0; -		} -
>> if (strncmp (buf, LOOP_SIGNATURE, strlen (LOOP_SIGNATURE)) != 0)
>> { -			free (buf); -			return 1; -                } -		memset
>> (buf, 0, strlen (LOOP_SIGNATURE)); -		return ped_device_write
>> (disk->dev, buf, 0, 1); -	} - memset (buf, 0, buflen); strcpy
>> (buf, LOOP_SIGNATURE);
>> 
>> -        int write_ok = ped_device_write (disk->dev, buf, 0, 1); 
>> -        free (buf); -	return write_ok; +        if
>> (ped_device_write (disk->dev, buf, 0, 1)) +		return 1; +	return
>> 0;
> 
> It looks like this leaks buf everywhere it returns.

alloca() allocates on the stack, so it is implicitly freed on return.
 It also can't fail, so no need for annoying tests and frees
everywhere for a temporary buffer.