[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