Package: parted;
Reported by: Phillip Susi <psusi <at> ubuntu.com>
Date: Mon, 23 Dec 2013 20:54:02 UTC
Severity: normal
Tags: patch
Done: Phillip Susi <psusi <at> ubuntu.com>
Bug is archived. No further changes may be made.
Message #23 received at 16231 <at> debbugs.gnu.org (full text, mbox):
From: Phillip Susi <psusi <at> ubuntu.com> To: "Brian C. Lane" <bcl <at> redhat.com>, 16231 <at> debbugs.gnu.org Subject: Re: bug#16231: [PATCH 1/2] Fix loop labels Date: Mon, 03 Mar 2014 19:30:43 -0500
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
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 <at> 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBCgAGBQJTFR6zAAoJEI5FoCIzSKrwuWoH/3MinQNcW9S5jEVyMlIFtT0X
hkaKRF15EO/B8B5MJwSiQ5fQ4yrjC0EScu7bGeB9UWMkFkWcuGntp3l+kTNrxGv6
oCBxTi2yqkb9E3/bo6FdDwFjrCM2JSE6ht1h4JVZsU+77knugrmRcGtIsVEgleLD
LD/wDEsjElZzu+TB+ixv0vgDKGXxPq+hjH2eJe416IeSDRMKe/dWZpAq6DGWc6Du
Bnfr4qWjFBdDFSUNtzKmTfVxvnk/Zbtt1hdBaNQ4SgerpjOthF3ACjxkAfRs5zP9
4L4AxPUzmygEdekGO4B205AU0sU8QT6FOyMMMhbSFkGwqjCBqoQxe3+Cy62Yfp8=
=hecx
-----END PGP SIGNATURE-----
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.