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

[Jim Meyering]

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.

I'll have to defer reviewing the rest to later today or tomorrow,
but wanted to give you some feedback: partial now is better than full later.