[parted-devel] [PATCH 1/6] libparted: refactor device-mapper partition sync code

[Jim Meyering]

Jim Meyering wrote:

> Phillip Susi wrote:
>> The device-mapper partition sync code was still using the remove all
>> partitions, then add new partitions method.  Refactor to use the same
>> algorithm as regular disks: try to remove all, and ignore any that could
>> not be removed but have not changed.
>> ---
>>  NEWS                   |    2 +
>>  libparted/arch/linux.c |  403 +++++++++++++++++++++---------------------------
>>  tests/Makefile.am      |    1 +
>>  tests/t6002-dm-busy.sh |   94 +++++++++++
>>  4 files changed, 275 insertions(+), 225 deletions(-)
>>  create mode 100644 tests/t6002-dm-busy.sh
>>
>> diff --git a/NEWS b/NEWS
>> index 4c4716d..f182cce 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -3,6 +3,8 @@ GNU parted NEWS -*- outline -*-
>>  * Noteworthy changes in release ?.? (????-??-??) [?]
>>
>>  ** Bug Fixes
>> +  libparted: Don't fail to manipulate partitions on dmraid disks that
>> +  have other partitions in use.
>
> Thanks for putting this together!
> It's great to see both NEWS and a test addition.
>
> ...
>> +static int
>> +_dm_add_partition (PedDisk* disk, const PedPartition* part)
>> +{
>> +        LinuxSpecific*  arch_specific = LINUX_SPECIFIC (disk->dev);
>> +
>> +        /* Get map name from devicemapper */
>> +        struct dm_task *task = dm_task_create (DM_DEVICE_INFO);
>> +        if (!task)
>> +                goto err;
>> +
>> +        if (!dm_task_set_major_minor (task, arch_specific->major,
>> +                                      arch_specific->minor, 0))
>> +                goto err;
>> +
>> +        if (!dm_task_run(task))
>> +                goto err;
>> +
>> +        const char *dev_name = dm_task_get_name (task);
>> +        char *vol_name;
>> +        if (isdigit (dev_name[strlen (dev_name) - 1])) {
>> +                if ( ! (vol_name = zasprintf ("%sp%d", dev_name, part->num)))
>> +                        goto err;
>> +        } else if ( ! (vol_name = zasprintf ("%s%d", dev_name, part->num)))
>> +                goto err;
>
> Please combine those:
>
>            char *vol_name
>              = zasprintf ("%s%s%d",
>                           dev_name,
>                           isdigit (dev_name[strlen (dev_name) - 1]) ? "p" : "",
>                           part->num);
>            if (vol_name == NULL)
>                    goto err;
>
>> +        /* Caution: dm_task_destroy frees dev_name.  */
>> +        dm_task_destroy (task);
>> +        task = NULL;
>> +	char *params;
>
> Please indent consistently.
> s/TAB/        /
> Yeah, I know it's hard, since the code does not use a consistent policy.
>
> Eventually, when there seem to be few distro patches anywhere,
> I'd like to convert all leading TABs to spaces.
>
>> +        if ( ! (params = zasprintf ("%d:%d %lld", arch_specific->major,
>> +                                    arch_specific->minor, part->geom.start)))
>> +                goto err;
>> +
>> +        task = dm_task_create (DM_DEVICE_CREATE);
>> +        if (!task)
>> +                goto err;
>> +
>> +        dm_task_set_name (task, vol_name);
>> +        dm_task_add_target (task, 0, part->geom.length,
>> +                "linear", params);
>> +        if (dm_task_run (task)) {
>> +                dm_task_update_nodes();
>> +                dm_task_destroy(task);
>> +                free(params);
>> +                free(vol_name);
>> +                return 1;
>> +        } else {
>> +                _dm_remove_partition (disk, part->num);
>> +        }
>> +err:
>> +        dm_task_update_nodes();
>> +        if (task)
>> +                dm_task_destroy (task);
>> +        free (params);
>> +        free (vol_name);
>> +        return 0;
>> +}
>> +#endif
>
> Please make a point always to configure with --enable-gcc-warnings.
> When I applied your patch and built, it highlights these two problems:
>
>       CC       arch/linux.lo
>     arch/linux.c: In function '_dm_add_partition':
>     arch/linux.c:2686:14: error: 'params' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>              free (params);
>                   ^
>     arch/linux.c:2687:14: error: 'vol_name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>              free (vol_name);
>                   ^
>     cc1: all warnings being treated as errors
>     make[3]: *** [arch/linux.lo] Error 1
>
> i.e., hitting any of the "goto err" statements before "params" is set
> would make libparted apply free to at least one uninitialized pointer.
>
> Also, there was a trailing space on one of the added lines,
> if you could remove it for a v2, that'd save me the trouble.

Looks like I'm doing a v2 for you.
Here's the first amendment, to fix the bugs highlighted by the errors
above:

diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index 0d5bad5..22556e8 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2634,6 +2634,8 @@ static int
 _dm_add_partition (PedDisk* disk, const PedPartition* part)
 {
         LinuxSpecific*  arch_specific = LINUX_SPECIFIC (disk->dev);
+        char *params = NULL;
+        char *vol_name = NULL;

         /* Get map name from devicemapper */
         struct dm_task *task = dm_task_create (DM_DEVICE_INFO);
@@ -2648,7 +2650,6 @@ _dm_add_partition (PedDisk* disk, const PedPartition* part)
                 goto err;

         const char *dev_name = dm_task_get_name (task);
-        char *vol_name;
         if (isdigit (dev_name[strlen (dev_name) - 1])) {
                 if ( ! (vol_name = zasprintf ("%sp%d", dev_name, part->num)))
                         goto err;
@@ -2658,7 +2659,6 @@ _dm_add_partition (PedDisk* disk, const PedPartition* part)
         /* Caution: dm_task_destroy frees dev_name.  */
         dm_task_destroy (task);
         task = NULL;
-	char *params;
         if ( ! (params = zasprintf ("%d:%d %lld", arch_specific->major,
                                     arch_specific->minor, part->geom.start)))
                 goto err;
@@ -2671,10 +2671,10 @@ _dm_add_partition (PedDisk* disk, const PedPartition* part)
         dm_task_add_target (task, 0, part->geom.length,
                 "linear", params);
         if (dm_task_run (task)) {
-                dm_task_update_nodes();
-                dm_task_destroy(task);
-                free(params);
-                free(vol_name);
+                dm_task_update_nodes ();
+                dm_task_destroy (task);
+                free (params);
+                free (vol_name);
                 return 1;
         } else {
                 _dm_remove_partition (disk, part->num);