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 #20 received at submit <at> debbugs.gnu.org (full text, mbox):
From: "Brian C. Lane" <bcl <at> redhat.com> To: bug-parted <at> gnu.org Subject: Re: bug#16231: [PATCH 1/2] Fix loop labels Date: Mon, 3 Mar 2014 10:48:57 -0800
On Fri, Jan 03, 2014 at 11:48:30PM -0500, Phillip Susi wrote:
> Loop labels were incorrectly identifying the device for the fictional
> partition as $dev1 instead of just $dev. This caused other programs like
> gparted to be confused, and caused parted to fail to identify the partition
> as busy due to the fact that it was looking for the wrong device. Parted
> also actually created the partition device so your raw fs on $dev gained an
> alias as $dev1. Next, writing the label back to the disk clobbered the
> filesystem there if it used the first sector. Several filesystems end up
> using the first sector for 2048/4096 byte sectors even though they don't
> for 512/1024 byte sectors. Finally, fat and ntfs boot sectors were being
> detected as msdos labels.
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.
Meanwhile I do have some comments about this code.
> ---
> NEWS | 2 +
> include/parted/device.in.h | 1 +
> libparted/arch/linux.c | 17 ++++++--
> libparted/disk.c | 2 +
> libparted/fs/ntfs/ntfs.c | 2 +-
> libparted/labels/dos.c | 29 ++++++++++++++
> libparted/labels/loop.c | 42 +++++++-------------
> partprobe/partprobe.c | 4 +-
> tests/Makefile.am | 1 +
> tests/t1102-loop-label.sh | 96 ++++++++++++++++++++++++++++++++++++++++++++++
> 10 files changed, 161 insertions(+), 35 deletions(-)
> create mode 100644 tests/t1102-loop-label.sh
I still think we should split up tests into their own commits.
>
> 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.
> @@ -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.
> 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.
> @@ -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.
--
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.