[libvirt] [PATCH v3] storage: Allow to delete device mapper disk partition

The name convention of device mapper disk is different, and 'parted' can't be used to delete a device mapper disk partition. e.g. Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 Error: Expecting a partition number. This patch introduces 'dmsetup' to fix it. Changes: - New function "virIsDevMapperDevice" in "src/utils/utils.c" - remove "is_dm_device" in "src/storage/parthelper.c", use "virIsDevMapperDevice" instead. - Requires "device-mapper" for 'with-storage-disk" in "libvirt.spec.in" - Check "dmsetup" in 'configure.ac' for "with-storage-disk" - Changes on "src/Makefile.am" to link against libdevmapper - New entry for "virIsDevMapperDevice" in "src/libvirt_private.syms" Changes from v1 to v3: - s/virIsDeviceMapperDevice/virIsDevMapperDevice/g - replace "virRun" with "virCommand" - sort the list of util functions in "libvirt_private.syms" - ATTRIBUTE_NONNULL(1) for virIsDevMapperDevice declaration. e.g. Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 Vol /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 deleted Name Path ----------------------------------------- --- configure.ac | 28 +++++++++++++++------ libvirt.spec.in | 1 + src/Makefile.am | 8 +++--- src/libvirt_private.syms | 2 +- src/storage/parthelper.c | 14 +--------- src/storage/storage_backend_disk.c | 48 +++++++++++++++++++++-------------- src/util/util.c | 15 +++++++++++ src/util/util.h | 2 + 8 files changed, 73 insertions(+), 45 deletions(-) diff --git a/configure.ac b/configure.ac index 3cd824a..44a3b19 100644 --- a/configure.ac +++ b/configure.ac @@ -1725,12 +1725,19 @@ LIBPARTED_LIBS= if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; then AC_PATH_PROG([PARTED], [parted], [], [$PATH:/sbin:/usr/sbin]) + AC_PATH_PROG([DMSETUP], [dmsetup], [], [$PATH:/sbin:/usr/sbin]) if test -z "$PARTED" ; then PARTED_FOUND=no else PARTED_FOUND=yes fi + if test -z "$DMSETUP" ; then + DMSETUP_FOUND=no + else + DMSETUP_FOUND=yes + fi + if test "$PARTED_FOUND" = "yes" && test "x$PKG_CONFIG" != "x" ; then PKG_CHECK_MODULES([LIBPARTED], [libparted >= $PARTED_REQUIRED], [], [PARTED_FOUND=no]) @@ -1748,14 +1755,17 @@ if test "$with_storage_disk" = "yes" || CFLAGS="$save_CFLAGS" fi - if test "$PARTED_FOUND" = "no" ; then - if test "$with_storage_disk" = "yes" ; then - AC_MSG_ERROR([We need parted for disk storage driver]) - else - with_storage_disk=no - fi - else - with_storage_disk=yes + if test "$with_storage_disk" = "yes" && + test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + AC_MSG_ERROR([Need both parted and dmsetup for disk storage driver]) + fi + + if test "$with_storage_disk" = "check"; then + if test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + with_storage_disk=no + else + with_storage_disk=yes + fi fi if test "$with_storage_disk" = "yes"; then @@ -1763,6 +1773,8 @@ if test "$with_storage_disk" = "yes" || [whether Disk backend for storage driver is enabled]) AC_DEFINE_UNQUOTED([PARTED],["$PARTED"], [Location or name of the parted program]) + AC_DEFINE_UNQUOTED([DMSETUP],["$DMSETUP"], + [Location or name of the dmsetup program]) fi fi AM_CONDITIONAL([WITH_STORAGE_DISK], [test "$with_storage_disk" = "yes"]) diff --git a/libvirt.spec.in b/libvirt.spec.in index 5021f45..8cb8fad 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -279,6 +279,7 @@ Requires: iscsi-initiator-utils %if %{with_storage_disk} # For disk driver Requires: parted +Requires: device-mapper %endif %if %{with_storage_mpath} # For multipath support diff --git a/src/Makefile.am b/src/Makefile.am index 2f94efd..6e14043 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -437,9 +437,9 @@ libvirt_la_BUILT_LIBADD = libvirt_util.la libvirt_util_la_SOURCES = \ $(UTIL_SOURCES) libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \ - $(AM_CFLAGS) $(AUDIT_CFLAGS) + $(AM_CFLAGS) $(AUDIT_CFLAGS) $(DEVMAPPER_CFLAGS) libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ - $(LIB_PTHREAD) $(AUDIT_LIBS) + $(LIB_PTHREAD) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) noinst_LTLIBRARIES += libvirt_conf.la @@ -1154,7 +1154,6 @@ libvirt_parthelper_SOURCES = $(STORAGE_HELPER_DISK_SOURCES) libvirt_parthelper_LDFLAGS = $(WARN_LDFLAGS) $(AM_LDFLAGS) libvirt_parthelper_LDADD = \ $(LIBPARTED_LIBS) \ - $(DEVMAPPER_LIBS) \ libvirt_util.la \ ../gnulib/lib/libgnu.la @@ -1179,7 +1178,8 @@ libvirt_lxc_SOURCES = \ libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(AM_LDFLAGS) libvirt_lxc_LDADD = $(CAPNG_LIBS) $(YAJL_LIBS) \ $(LIBXML_LIBS) $(NUMACTL_LIBS) $(LIB_PTHREAD) \ - $(LIBNL_LIBS) $(AUDIT_LIBS) ../gnulib/lib/libgnu.la + $(LIBNL_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ + ../gnulib/lib/libgnu.la libvirt_lxc_CFLAGS = \ $(LIBPARTED_CFLAGS) \ $(NUMACTL_CFLAGS) \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b9e3efe..1ab3da3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -884,6 +884,7 @@ virGetUserID; virGetUserName; virHexToBin; virIndexToDiskName; +virIsDevMapperDevice; virKillProcess; virMacAddrCompare; virParseMacAddr; @@ -910,7 +911,6 @@ virStrncpy; virTimestamp; virVasprintf; - # uuid.h virGetHostUUID; virSetHostUUIDStr; diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 6ef413d..acc9171 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -59,18 +59,6 @@ enum diskCommand { DISK_GEOMETRY }; -static int -is_dm_device(const char *devname) -{ - struct stat buf; - - if (devname && !stat(devname, &buf) && dm_is_dm_major(major(buf.st_rdev))) { - return 1; - } - - return 0; -} - int main(int argc, char **argv) { PedDevice *dev; @@ -96,7 +84,7 @@ int main(int argc, char **argv) } path = argv[1]; - if (is_dm_device(path)) { + if (virIsDevMapperDevice(path)) { partsep = "p"; canonical_path = strdup(path); if (canonical_path == NULL) { diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index c7ade6b..70e3ba6 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -31,6 +31,7 @@ #include "storage_backend_disk.h" #include "util.h" #include "memory.h" +#include "command.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -647,6 +648,8 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, char *part_num = NULL; char *devpath = NULL; char *devname, *srcname; + virCommandPtr cmd = NULL; + bool isDevMapperDevice; int rc = -1; if (virFileResolveLink(vol->target.path, &devpath) < 0) { @@ -660,36 +663,43 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, srcname = basename(pool->def->source.devices[0].path); DEBUG("devname=%s, srcname=%s", devname, srcname); - if (!STRPREFIX(devname, srcname)) { + isDevMapperDevice = virIsDevMapperDevice(devpath); + + if (!isDevMapperDevice && !STRPREFIX(devname, srcname)) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Volume path '%s' did not start with parent " "pool source device name."), devname); goto cleanup; } - part_num = devname + strlen(srcname); + if (!isDevMapperDevice) { + part_num = devname + strlen(srcname); - if (*part_num == 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse partition number from target " - "'%s'"), devname); - goto cleanup; - } + if (*part_num == 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse partition number from target " + "'%s'"), devname); + goto cleanup; + } - /* eg parted /dev/sda rm 2 */ - const char *prog[] = { - PARTED, - pool->def->source.devices[0].path, - "rm", - "--script", - part_num, - NULL, - }; + cmd = virCommandNewArgList(PARTED, + pool->def->source.devices[0].path, + "rm", + "--script", + part_num, + NULL); - if (virRun(prog, NULL) < 0) - goto cleanup; + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + } else { + cmd = virCommandNewArgList(DMSETUP, "remove", "--force", devpath, NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + } rc = 0; cleanup: VIR_FREE(devpath); + virCommandFree(cmd); return rc; diff --git a/src/util/util.c b/src/util/util.c index 6161be6..1ec61b8 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -45,6 +45,7 @@ #include <string.h> #include <signal.h> #include <termios.h> +#include <libdevmapper.h> #include "c-ctype.h" #ifdef HAVE_PATHS_H @@ -3123,3 +3124,17 @@ virTimestamp(void) return timestamp; } + +bool +virIsDevMapperDevice(const char *devname) +{ + struct stat buf; + + if (!stat(devname, &buf) && + S_ISBLK(buf.st_mode) && + dm_is_dm_major(major(buf.st_rdev))) + return true; + + return false; +} + diff --git a/src/util/util.h b/src/util/util.h index 8373038..10d3253 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -296,4 +296,6 @@ int virBuildPathInternal(char **path, ...) ATTRIBUTE_SENTINEL; char *virTimestamp(void); +bool virIsDevMapperDevice(const char *devname) ATTRIBUTE_NONNULL(1); + #endif /* __VIR_UTIL_H__ */ -- 1.7.4

On Sat, Feb 12, 2011 at 03:36:28PM +0800, Osier Yang wrote:
The name convention of device mapper disk is different, and 'parted' can't be used to delete a device mapper disk partition. e.g.
Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1
Error: Expecting a partition number.
This patch introduces 'dmsetup' to fix it.
Changes: - New function "virIsDevMapperDevice" in "src/utils/utils.c" - remove "is_dm_device" in "src/storage/parthelper.c", use "virIsDevMapperDevice" instead. - Requires "device-mapper" for 'with-storage-disk" in "libvirt.spec.in" - Check "dmsetup" in 'configure.ac' for "with-storage-disk" - Changes on "src/Makefile.am" to link against libdevmapper - New entry for "virIsDevMapperDevice" in "src/libvirt_private.syms"
Changes from v1 to v3: - s/virIsDeviceMapperDevice/virIsDevMapperDevice/g - replace "virRun" with "virCommand" - sort the list of util functions in "libvirt_private.syms" - ATTRIBUTE_NONNULL(1) for virIsDevMapperDevice declaration.
e.g.
Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1
Vol /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 deleted
This looks fine to me, but since Eric reviewed the patch before and had issues I will let him give the final ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 02/12/2011 12:36 AM, Osier Yang wrote:
The name convention of device mapper disk is different, and 'parted' can't be used to delete a device mapper disk partition. e.g.
Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1
Error: Expecting a partition number.
This patch introduces 'dmsetup' to fix it.
@@ -1748,14 +1755,17 @@ if test "$with_storage_disk" = "yes" || CFLAGS="$save_CFLAGS" fi
- if test "$PARTED_FOUND" = "no" ; then - if test "$with_storage_disk" = "yes" ; then - AC_MSG_ERROR([We need parted for disk storage driver]) - else - with_storage_disk=no - fi - else - with_storage_disk=yes + if test "$with_storage_disk" = "yes" && + test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + AC_MSG_ERROR([Need both parted and dmsetup for disk storage driver]) + fi + + if test "$with_storage_disk" = "check"; then + if test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + with_storage_disk=no + else + with_storage_disk=yes + fi
This configure logic looks much better; thanks for persisting. ACK; however, since this changes configure.ac and shuffles some link libraries around in Makefile.am, I'd feel more comfortable postponing this until after 0.8.8 is released, so that we don't introduce any unintended compile failures compared to the release candidate. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

于 2011年02月15日 00:27, Eric Blake 写道:
On 02/12/2011 12:36 AM, Osier Yang wrote:
The name convention of device mapper disk is different, and 'parted' can't be used to delete a device mapper disk partition. e.g.
Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1
Error: Expecting a partition number.
This patch introduces 'dmsetup' to fix it.
@@ -1748,14 +1755,17 @@ if test "$with_storage_disk" = "yes" || CFLAGS="$save_CFLAGS" fi
- if test "$PARTED_FOUND" = "no" ; then - if test "$with_storage_disk" = "yes" ; then - AC_MSG_ERROR([We need parted for disk storage driver]) - else - with_storage_disk=no - fi - else - with_storage_disk=yes + if test "$with_storage_disk" = "yes"&& + test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + AC_MSG_ERROR([Need both parted and dmsetup for disk storage driver]) + fi + + if test "$with_storage_disk" = "check"; then + if test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + with_storage_disk=no + else + with_storage_disk=yes + fi
This configure logic looks much better; thanks for persisting.
ACK; however, since this changes configure.ac and shuffles some link libraries around in Makefile.am, I'd feel more comfortable postponing this until after 0.8.8 is released, so that we don't introduce any unintended compile failures compared to the release candidate.
Ok, thanks for the revewing, I'm pushing it later than 0.8.8 is released. Regards Osier

于 2011年02月15日 10:02, Osier Yang 写道:
于 2011年02月15日 00:27, Eric Blake 写道:
On 02/12/2011 12:36 AM, Osier Yang wrote:
The name convention of device mapper disk is different, and 'parted' can't be used to delete a device mapper disk partition. e.g.
Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1
Error: Expecting a partition number.
This patch introduces 'dmsetup' to fix it.
@@ -1748,14 +1755,17 @@ if test "$with_storage_disk" = "yes" || CFLAGS="$save_CFLAGS" fi
- if test "$PARTED_FOUND" = "no" ; then - if test "$with_storage_disk" = "yes" ; then - AC_MSG_ERROR([We need parted for disk storage driver]) - else - with_storage_disk=no - fi - else - with_storage_disk=yes + if test "$with_storage_disk" = "yes"&& + test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + AC_MSG_ERROR([Need both parted and dmsetup for disk storage driver]) + fi + + if test "$with_storage_disk" = "check"; then + if test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + with_storage_disk=no + else + with_storage_disk=yes + fi
This configure logic looks much better; thanks for persisting.
ACK; however, since this changes configure.ac and shuffles some link libraries around in Makefile.am, I'd feel more comfortable postponing this until after 0.8.8 is released, so that we don't introduce any unintended compile failures compared to the release candidate.
Ok, thanks for the revewing, I'm pushing it later than 0.8.8 is released.
As 0.8.8 is released, rebased and pushed. Regards Osier
participants (3)
-
Daniel Veillard
-
Eric Blake
-
Osier Yang