[libvirt] [PATCH] Make logical pools independent on target path

Osier Yang jyang at redhat.com
Mon Jun 3 16:22:28 UTC 2013


On 03/06/13 23:05, Martin Kletzander wrote:
> When using logical pools, we had to trust the target->pth provided.

s/pth/path/,

> This parameter, however, can be completely ommited and we can get the
> correct path using 'lvdisplay' command.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=952973
>
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>
> Notes:
>      I'm not sure we can drop the target.path from the XML (see tests), so
>      another variant is to keep it there the same way it was defined by
>      user/mgmt app.
How about one file a bug "The created volume is not under specified pool's
target path"? Since with this patch, the specified pool's target path is 
silently
ignored.

If you have a good answer/reason/fix for above question, I see no problem of
ignore the pool->target.path, but the path you got from lvdisplay (with 
vol name
is truncated) should be reflected to the pool's XML.

>   If that's desired, mentioning it with an ack is enough
>      for me to know I should drop the conf/storage_conf.c hunk as well as
>      the hunks patching tests.
>
>   configure.ac                                       |  4 ++
>   src/conf/storage_conf.c                            | 19 +++---
>   src/storage/storage_backend_logical.c              | 79 +++++++++++++++-------
>   tests/storagepoolxml2xmlin/pool-logical-create.xml |  1 -
>   tests/storagepoolxml2xmlin/pool-logical.xml        |  1 -
>   .../storagepoolxml2xmlout/pool-logical-create.xml  |  1 -
>   tests/storagepoolxml2xmlout/pool-logical.xml       |  1 -
>   7 files changed, 67 insertions(+), 39 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 5d1bc6b..753139d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1603,6 +1603,7 @@ if test "$with_storage_lvm" = "yes" || test "$with_storage_lvm" = "check"; then
>     AC_PATH_PROG([PVS], [pvs], [], [$PATH:/sbin:/usr/sbin])
>     AC_PATH_PROG([VGS], [vgs], [], [$PATH:/sbin:/usr/sbin])
>     AC_PATH_PROG([LVS], [lvs], [], [$PATH:/sbin:/usr/sbin])
> +  AC_PATH_PROG([LVDISPLAY], [lvdisplay], [], [$PATH:/sbin:/usr/sbin])
>
>     if test "$with_storage_lvm" = "yes" ; then
>       if test -z "$PVCREATE" ; then AC_MSG_ERROR([We need pvcreate for LVM storage driver]) ; fi
> @@ -1617,6 +1618,7 @@ if test "$with_storage_lvm" = "yes" || test "$with_storage_lvm" = "check"; then
>       if test -z "$PVS" ; then AC_MSG_ERROR([We need pvs for LVM storage driver]) ; fi
>       if test -z "$VGS" ; then AC_MSG_ERROR([We need vgs for LVM storage driver]) ; fi
>       if test -z "$LVS" ; then AC_MSG_ERROR([We need lvs for LVM storage driver]) ; fi
> +    if test -z "$LVDISPLAY" ; then AC_MSG_ERROR([We need lvdisplay for LVM storage driver]) ; fi
>     else
>       if test -z "$PVCREATE" ; then with_storage_lvm=no ; fi
>       if test -z "$VGCREATE" ; then with_storage_lvm=no ; fi
> @@ -1630,6 +1632,7 @@ if test "$with_storage_lvm" = "yes" || test "$with_storage_lvm" = "check"; then
>       if test -z "$PVS" ; then with_storage_lvm=no ; fi
>       if test -z "$VGS" ; then with_storage_lvm=no ; fi
>       if test -z "$LVS" ; then with_storage_lvm=no ; fi
> +    if test -z "$LVDISPLAY" ; then with_storage_lvm=no ; fi
>
>       if test "$with_storage_lvm" = "check" ; then with_storage_lvm=yes ; fi
>     fi
> @@ -1648,6 +1651,7 @@ if test "$with_storage_lvm" = "yes" || test "$with_storage_lvm" = "check"; then
>       AC_DEFINE_UNQUOTED([PVS],["$PVS"],[Location of pvs program])
>       AC_DEFINE_UNQUOTED([VGS],["$VGS"],[Location of vgs program])
>       AC_DEFINE_UNQUOTED([LVS],["$LVS"],[Location of lvs program])
> +    AC_DEFINE_UNQUOTED([LVDISPLAY],["$LVDISPLAY"],[Location of lvdisplay program])
>     fi
>   fi
>   AM_CONDITIONAL([WITH_STORAGE_LVM], [test "$with_storage_lvm" = "yes"])
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index cc3d3d9..948e190 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1,7 +1,7 @@
>   /*
>    * storage_conf.c: config handling for storage driver
>    *
> - * Copyright (C) 2006-2012 Red Hat, Inc.
> + * Copyright (C) 2006-2013 Red Hat, Inc.
>    * Copyright (C) 2006-2008 Daniel P. Berrange
>    *
>    * This library is free software; you can redistribute it and/or
> @@ -947,15 +947,16 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>       /* When we are working with a virtual disk we can skip the target
>        * path and permissions */
>       if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) {
> -        if (!(target_path = virXPathString("string(./target/path)", ctxt))) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("missing storage pool target path"));
> -            goto error;
> +        if (ret->type != VIR_STORAGE_POOL_LOGICAL) {
> +            if (!(target_path = virXPathString("string(./target/path)", ctxt))) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("missing storage pool target path"));
> +                goto error;
> +            }
> +            ret->target.path = virFileSanitizePath(target_path);
> +            if (!ret->target.path)
> +                goto error;

RNG schema needs to be changed too..

>           }
> -        ret->target.path = virFileSanitizePath(target_path);
> -        if (!ret->target.path)
> -            goto error;
> -
>           if (virStorageDefParsePerms(ctxt, &ret->target.perms,
>                                       "./target/permissions",
>                                       DEFAULT_POOL_PERM_MODE) < 0)
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 944aa0e..66a4e42 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -45,6 +45,37 @@
>   #define PV_BLANK_SECTOR_SIZE 512
>
>
> +static char *
> +virStorageBackendGetVolPath(const char *poolname, const char *volname)
> +{
> +    char *start = NULL;
> +    char *lvpath = NULL;
> +    char *output = NULL;
> +
> +    virCommandPtr cmd = virCommandNewArgList(LVDISPLAY,
> +                                             "--columns",
> +                                             "--options", "lv_path",
> +                                             "--noheadings",
> +                                             "--unbuffered",
> +                                             NULL);
> +
> +    virCommandAddArgFormat(cmd, "%s/%s", poolname, volname);
> +    virCommandSetOutputBuffer(cmd, &output);
> +
> +    if (virCommandRun(cmd, NULL) < 0 ||
> +        !(start = strchr(output, '/'))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get LV path"));
> +        goto cleanup;
> +    }
> +
> +    ignore_value(VIR_STRDUP(lvpath, start));
> +
> + cleanup:
> +    VIR_FREE(output);
> +    virCommandFree(cmd);
> +    return lvpath;
> +}
> +
>   static int
>   virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
>                                     int on)
> @@ -116,11 +147,8 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
>       }
>
>       if (vol->target.path == NULL) {
> -        if (virAsprintf(&vol->target.path, "%s/%s",
> -                        pool->def->target.path, vol->name) < 0) {
> -            virReportOOMError();
> +        if (VIR_STRDUP(vol->target.path, groups[9]) < 0)
>               goto cleanup;
> -        }
>       }
>
>       /* Skips the backingStore of lv created with "--virtualsize",
> @@ -130,12 +158,11 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
>        * (lvs outputs "[$lvname_vorigin] for field "origin" if the
>        *  lv is created with "--virtualsize").
>        */
> -    if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] != '[')) {
> -        if (virAsprintf(&vol->backingStore.path, "%s/%s",
> -                        pool->def->target.path, groups[1]) < 0) {
> -            virReportOOMError();
> +    if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] != '[')) {
> +        vol->backingStore.path = virStorageBackendGetVolPath(pool->def->source.name,
> +                                                             groups[1]);
> +        if (!vol->backingStore.path)
>               goto cleanup;
> -        }
>
>           vol->backingStore.format = VIR_STORAGE_POOL_LOGICAL_LVM2;
>       }
> @@ -277,21 +304,20 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
>                                   virStorageVolDefPtr vol)
>   {
>       /*
> -     * # lvs --separator , --noheadings --units b --unbuffered --nosuffix --options \
> -     * "lv_name,origin,uuid,devices,seg_size,vg_extent_size,size,lv_attr" VGNAME
> +     * # lvs --separator '#' --noheadings --units b --unbuffered --nosuffix --options "lv_name,origin,uuid,devices,seg_size,vg_extent_size,size,lv_path" VGNAME
>        *
> -     * RootLV,,06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky,/dev/hda2(0),5234491392,33554432,5234491392,-wi-ao
> -     * SwapLV,,oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M,/dev/hda2(156),1040187392,33554432,1040187392,-wi-ao
> -     * Test2,,3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR,/dev/hda2(219),1073741824,33554432,1073741824,owi-a-
> -     * Test3,,UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht,/dev/hda2(251),2181038080,33554432,2181038080,-wi-a-
> -     * Test3,Test2,UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht,/dev/hda2(187),1040187392,33554432,1040187392,swi-a-
> +     * RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#5234491392#33554432#5234491392#/dev/VGNAME/RootLV
> +     * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#1040187392#33554432#1040187392#/dev/VGNAME/SwapLV
> +     * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#1073741824#33554432#1073741824#/dev/VGNAME/Test2
> +     * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#2181038080#33554432#2181038080#/dev/VGNAME/Test3
> +     * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#1040187392#33554432#1040187392#/dev/VGNAME/Test3
>        *
>        * Pull out name, origin, & uuid, device, device extent start #,
>        * segment size, extent size, size, attrs
>        *
>        * NB can be multiple rows per volume if they have many extents
>        *
> -     * NB lvs from some distros (e.g. SLES10 SP2) outputs trailing "," on each line
> +     * NB lvs from some distros (e.g. SLES10 SP2) outputs trailing separator on each line
>        *
>        * NB Encrypted logical volumes can print ':' in their name, so it is
>        *    not a suitable separator (rhbz 470693).
> @@ -314,7 +340,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
>                                  "--unbuffered",
>                                  "--nosuffix",
>                                  "--options",
> -                               "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr",

I won't want to see what I added (lv_attr) is removed by mistake, :-)

> +                               "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_path",
>                                  pool->def->source.name,
>                                  NULL);
>       if (virStorageBackendRunProgRegex(pool,
> @@ -702,6 +728,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>       int fd = -1;
>       virCommandPtr cmd = NULL;
>       virErrorPtr err;
> +    bool created = false;
>
>       if (vol->target.encryption != NULL) {
>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -717,13 +744,6 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>           VIR_FREE(vol->target.path);
>       }
>
> -    if (virAsprintf(&vol->target.path, "%s/%s",
> -                    pool->def->target.path,
> -                    vol->name) == -1) {
> -        virReportOOMError();
> -        return -1;
> -    }
> -
>       cmd = virCommandNewArgList(LVCREATE,
>                                  "--name", vol->name,
>                                  NULL);
> @@ -742,9 +762,15 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>       if (virCommandRun(cmd, NULL) < 0)
>           goto error;
>
> +    created = true;
>       virCommandFree(cmd);
>       cmd = NULL;
>
> +    vol->target.path = virStorageBackendGetVolPath(pool->def->source.name,
> +                                                   vol->name);
> +    if (!vol->target.path)
> +        goto error;
> +
>       if ((fd = virStorageBackendVolOpen(vol->target.path)) < 0)
>           goto error;
>
> @@ -784,7 +810,8 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>    error:
>       err = virSaveLastError();
>       VIR_FORCE_CLOSE(fd);
> -    virStorageBackendLogicalDeleteVol(conn, pool, vol, 0);
> +    if (created)
> +        virStorageBackendLogicalDeleteVol(conn, pool, vol, 0);
>       virCommandFree(cmd);
>       virSetError(err);
>       virFreeError(err);
> diff --git a/tests/storagepoolxml2xmlin/pool-logical-create.xml b/tests/storagepoolxml2xmlin/pool-logical-create.xml
> index 4c67089..e1bb4db 100644
> --- a/tests/storagepoolxml2xmlin/pool-logical-create.xml
> +++ b/tests/storagepoolxml2xmlin/pool-logical-create.xml
> @@ -10,7 +10,6 @@
>       <device path="/dev/sdb3"/>
>     </source>
>     <target>
> -    <path>/dev/HostVG</path>

At least one test should have the specified target path, to make sure 
the old XML still work..

Osier




More information about the libvir-list mailing list