[parted-devel] Bug: Removal of BLKPG causes regression of ability to manipulate disks with other partitions in use

[Jim Meyering]

Phillip Susi wrote:

> On 3/30/2010 3:58 AM, Jim Meyering wrote:
>> I'll make the request once again:
>> Please give details of precisely how you test this (I assume you do).
>> Best is to give a script like one of those in tests/, but a
>> sequence of parted commands would be fine, too.
>> This will require root permissions and the use of the scsi_debug module,
>> so you may find it easiest to base a test script on one of the
>> existing 5 that do that:
>>   tests/t3000-resize-fs.sh
>>   tests/t3200-type-change.sh
>>   tests/t9010-big-sector.sh
>>   tests/t9020-alignment.sh
>>   tests/t9030-align-check.sh
>
> I have not looked at the automated testing at all.  I tested parted
> interactively on a real disk by adding and removing partitions while
> sometimes forcing one or more to be in use by spawning a dd process with
> a command like:
>
> dd of=fifo < /dev/sdd
>
> Which caused the shell to open the drive then dd blocks trying to open a
> named pipe I created with mkfifo.  I made sure to test removing the
> first and last partition both unused and while the partition next to it
> was in use.  I tested both msdos and gpt partition tables, and in the
> case of msdos, made sure to test the case where removing an extended
> partition would renumber the next one which was in use.  This last case
> resulted in the warning and the open partition remained with the higher
> number for the time being.  Subsequently closing the partition and
> either having fdisk force a BLKRRPART or using parted to make further
> changes with BLKPG successfully refreshed the kernel partition table.
>
> Oh that reminds me, is there a way to force the partition number used
> with mkpart instead of choosing the first number not in use already?  I
> ended up accidentally doing some testing with higher numbered partitions
> without the lower ones since I had already removed them and thought it
> might be a good test case to be able to artificially create say,
> partition 27 on a gpt disk by itself.
>
>>> +                                /* get start and length of existing partition */
>>> +                                dev_name = _device_get_part_path (disk->dev, i);
>>
>> Don't segfault when dev_name is NULL.
>
> How can it be NULL,

Just look at the definition of _device_get_part_path:

    static char*
    _device_get_part_path (PedDevice* dev, int num)
    {
            int             path_len = strlen (dev->path);
            int             result_len = path_len + 16;
            char*           result;

            result = (char*) ped_malloc (result_len);
            if (!result)
                    return NULL;


> and what would be the proper recovery?  Throw a bug
> exception and return?

I haven't looked closely yet, but that sounds right.
Though if you're returning early, be sure to free anything
that needs freeing.

>>> +                                fd = open (dev_name, O_RDONLY);
>>
>> You must free dev_name here, to avoid a leak.
>
> Oh, I didn't realize _device_get_part_path() duplicated the string, I
> thought it just returned the existing copy which would be freed when the
> device object was.

I guess this means you did look at the above function definition.