[libvirt] [PATCH] storage: Allow to delete device mapper disk partition
From: root <root@amd-1352-8-2.englab.nay.redhat.com> The name convention of device mapper disk is different, and 'parted' can't be used to delete a device mapper disk partition. e.g. [root@amd-1352-8-2 ~]# virsh vol-list --pool osier Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 [root@amd-1352-8-2 ~]# parted /dev/mapper/3600a0b80005adb0b0000ab2d4cae9254 rm p1 Error: Expecting a partition number. This patch introduces 'dmsetup' to fix it. Changes: - New function 'virIsDeviceMapperDevice' in src/utils/utils.c - Delete 'is_dm_device' in src/storage/parthelper.c, use 'virIsDeviceMapperDevice' 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 'virIsDeviceMapperDevice' in src/libvirt_private.syms e.g. [root@amd-1352- ~]# virsh vol-list --pool osier Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 [root@amd-1352- ~]# virsh vol-delete /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 Vol /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 deleted [root@amd-1352- ~]# virsh vol-list --pool osier Name Path ----------------------------------------- --- configure.ac | 21 ++++++++++++++ libvirt.spec.in | 1 + src/Makefile.am | 7 ++-- src/libvirt_private.syms | 1 + src/storage/parthelper.c | 14 +-------- src/storage/storage_backend_disk.c | 54 ++++++++++++++++++++++------------- src/util/util.c | 14 +++++++++ src/util/util.h | 1 + 8 files changed, 76 insertions(+), 37 deletions(-) diff --git a/configure.ac b/configure.ac index f310a5e..b101faa 100644 --- a/configure.ac +++ b/configure.ac @@ -1705,6 +1705,8 @@ LIBPARTED_CFLAGS= 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 with_storage_disk=no PARTED_FOUND=no @@ -1712,9 +1714,17 @@ if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; the PARTED_FOUND=yes fi + if test -z "$DMSETUP" ; then + with_storage_disk=no + DMSETUP_FOUND=no + else + DMSETUP_FOUND=yes + fi + if test "$with_storage_disk" != "no" && test "x$PKG_CONFIG" != "x" ; then PKG_CHECK_MODULES(LIBPARTED, libparted >= $PARTED_REQUIRED, [], [PARTED_FOUND=no]) fi + if test "$PARTED_FOUND" = "no"; then # RHEL-5 vintage parted is missing pkg-config files save_LIBS="$LIBS" @@ -1738,9 +1748,20 @@ if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; the with_storage_disk=yes fi + if test "$DMSETUP_FOUND" = "no"; then + if test "$with_storage_disk" = "yes" ; then + AC_MSG_ERROR([We need dmsetup for disk storage driver]) + else + with_storage_disk=no + fi + else + with_storage_disk=yes + fi + if test "$with_storage_disk" = "yes"; then AC_DEFINE_UNQUOTED([WITH_STORAGE_DISK], 1, [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 0a2d10e..27e6a99 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 f8b8434..3680074 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_LIBS) 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,7 @@ 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 3a5aec9..9d974e8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -900,6 +900,7 @@ virStrcpy; virStrncpy; virTimestamp; virVasprintf; +virIsDeviceMapperDevice; # uuid.h diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 6ef413d..1f662fb 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 (virIsDeviceMapperDevice(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..45e43c4 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -660,34 +660,48 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, srcname = basename(pool->def->source.devices[0].path); DEBUG("devname=%s, srcname=%s", devname, srcname); - if (!STRPREFIX(devname, srcname)) { + if (!virIsDeviceMapperDevice(devpath) && !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 (!virIsDeviceMapperDevice(devpath)) { + part_num = devname + strlen(srcname); - 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, - }; + if (*part_num == 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse partition number from target " + "'%s'"), devname); + goto cleanup; + } - if (virRun(prog, NULL) < 0) - goto cleanup; + /* eg parted /dev/sda rm 2 */ + const char *prog[] = { + PARTED, + pool->def->source.devices[0].path, + "rm", + "--script", + part_num, + NULL, + }; + + if (virRun(prog, NULL) < 0) + goto cleanup; + } else { + const char *prog[] = { + DMSETUP, + "remove", + "-f", + devpath, + NULL, + NULL, + }; + + if (virRun(prog, NULL) < 0) + goto cleanup; + } rc = 0; cleanup: diff --git a/src/util/util.c b/src/util/util.c index ee04ca9..6e2f966 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 @@ -3100,3 +3101,16 @@ virTimestamp(void) return timestamp; } + +int +virIsDeviceMapperDevice(const char *devname) +{ + struct stat buf; + + if (devname && !stat(devname, &buf) && dm_is_dm_major(major(buf.st_rdev))) { + return 1; + } + + return 0; +} + diff --git a/src/util/util.h b/src/util/util.h index 8373038..dd683ea 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -296,4 +296,5 @@ int virBuildPathInternal(char **path, ...) ATTRIBUTE_SENTINEL; char *virTimestamp(void); +int virIsDeviceMapperDevice(const char *devname); #endif /* __VIR_UTIL_H__ */ -- 1.7.1
On 01/31/2011 03:16 AM, Osier Yang wrote:
From: root
Fix your ~/.gitconfig, then 'git commit --amend --author=Osier' to reclaim authorship of this file; this patch is showing up with the wrong author information, and even though we could .mailmap around a bad commit, I'd rather not have bogus commit information in the first place.
This patch introduces 'dmsetup' to fix it.
How portable is dmsetup to other Linux distros? (Well, at least Fedora 14 and Ubuntu 10.10 had it without me doing anything, so I'm a bit reassured).
diff --git a/configure.ac b/configure.ac index f310a5e..b101faa 100644 --- a/configure.ac +++ b/configure.ac @@ -1705,6 +1705,8 @@ LIBPARTED_CFLAGS= 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])
At least you aren't making things any worse - the storage manager already depended on external programs only likely to be found on Linux.
+ if test -z "$PARTED" ; then with_storage_disk=no PARTED_FOUND=no
Hmm, pre-existing bug. If $with_storage_disk is yes, but $PARTED is not found, then we slam $with_storage_disk to no...
@@ -1712,9 +1714,17 @@ if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; the PARTED_FOUND=yes fi
+ if test -z "$DMSETUP" ; then + with_storage_disk=no + DMSETUP_FOUND=no + else + DMSETUP_FOUND=yes + fi + if test "$with_storage_disk" != "no" && test "x$PKG_CONFIG" != "x" ; then PKG_CHECK_MODULES(LIBPARTED, libparted >= $PARTED_REQUIRED, [], [PARTED_FOUND=no]) fi + if test "$PARTED_FOUND" = "no"; then # RHEL-5 vintage parted is missing pkg-config files save_LIBS="$LIBS" @@ -1738,9 +1748,20 @@ if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; the with_storage_disk=yes fi
+ if test "$DMSETUP_FOUND" = "no"; then + if test "$with_storage_disk" = "yes" ; then + AC_MSG_ERROR([We need dmsetup for disk storage driver])
but then later check if PARTED_FOUND is no (as copied by you to DMSETUP_FOUND begin no), we are still looking for with_storage_disk to be yes, which it can't be. Let's fix that logic bug first, before you copy and paste it to occur twice.
+++ b/src/libvirt_private.syms @@ -900,6 +900,7 @@ virStrcpy; virStrncpy; virTimestamp; virVasprintf; +virIsDeviceMapperDevice;
Keep this list sorted. This new line should be between virIndexToDiskName and virKillProcess.
+ } else { + const char *prog[] = { + DMSETUP, + "remove", + "-f", + devpath, + NULL, + NULL, + }; + + if (virRun(prog, NULL) < 0)
We probably ought to switch these over to virCommand, as long as we're touching the code.
+ +int +virIsDeviceMapperDevice(const char *devname) +{ + struct stat buf; + + if (devname && !stat(devname, &buf) && dm_is_dm_major(major(buf.st_rdev))) {
Phew - I was about to panic about licensing concerns, but it looks like you're okay - while the devmapper package consists of both LPGLv2 and GPLv2 portions, further reading shows that it is dmsetup that is GPLv2 (and external execution is fine), while the library is LGPLv2 (so moving the call to dm_* out of parthelper into the primary libvirt library is acceptable).
+int virIsDeviceMapperDevice(const char *devname);
s/int/bool/ as long as you are renaming the function. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* configure.ac (PARTED_FOUND): Issue configure error if --with-storage-disk=yes but no parted is found. --- As pointed out while reviewing Osier's patch, we were unconditionally nuking with_storage_disk=no if parted was not found, then trying to issue AC_MSG_ERROR if the user had passed in yes. Also break some long lines. configure.ac | 15 +++++++++------ 1 files changed, 9 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index f310a5e..2873fff 100644 --- a/configure.ac +++ b/configure.ac @@ -1703,17 +1703,18 @@ AC_SUBST([DEVMAPPER_LIBS]) LIBPARTED_CFLAGS= LIBPARTED_LIBS= -if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; then +if test "$with_storage_disk" = "yes" || + test "$with_storage_disk" = "check"; then AC_PATH_PROG([PARTED], [parted], [], [$PATH:/sbin:/usr/sbin]) if test -z "$PARTED" ; then - with_storage_disk=no PARTED_FOUND=no else PARTED_FOUND=yes fi - if test "$with_storage_disk" != "no" && test "x$PKG_CONFIG" != "x" ; then - PKG_CHECK_MODULES(LIBPARTED, libparted >= $PARTED_REQUIRED, [], [PARTED_FOUND=no]) + if test "$PARTED_FOUND" = yes && test "x$PKG_CONFIG" != "x" ; then + PKG_CHECK_MODULES(LIBPARTED, libparted >= $PARTED_REQUIRED, [], + [PARTED_FOUND=no]) fi if test "$PARTED_FOUND" = "no"; then # RHEL-5 vintage parted is missing pkg-config files @@ -1739,8 +1740,10 @@ if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; the fi if test "$with_storage_disk" = "yes"; then - AC_DEFINE_UNQUOTED([WITH_STORAGE_DISK], 1, [whether Disk backend for storage driver is enabled]) - AC_DEFINE_UNQUOTED([PARTED],["$PARTED"], [Location or name of the parted program]) + AC_DEFINE_UNQUOTED([WITH_STORAGE_DISK], 1, + [whether Disk backend for storage driver is enabled]) + AC_DEFINE_UNQUOTED([PARTED],["$PARTED"], + [Location or name of the parted program]) fi fi AM_CONDITIONAL([WITH_STORAGE_DISK], [test "$with_storage_disk" = "yes"]) -- 1.7.3.5
On 01/31/2011 05:10 PM, Eric Blake wrote:
* configure.ac (PARTED_FOUND): Issue configure error if --with-storage-disk=yes but no parted is found. ---
As pointed out while reviewing Osier's patch, we were unconditionally nuking with_storage_disk=no if parted was not found, then trying to issue AC_MSG_ERROR if the user had passed in yes. Also break some long lines.
configure.ac | 15 +++++++++------ 1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/configure.ac b/configure.ac index f310a5e..2873fff 100644 --- a/configure.ac +++ b/configure.ac @@ -1703,17 +1703,18 @@ AC_SUBST([DEVMAPPER_LIBS])
LIBPARTED_CFLAGS= LIBPARTED_LIBS= -if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; then +if test "$with_storage_disk" = "yes" || + test "$with_storage_disk" = "check"; then AC_PATH_PROG([PARTED], [parted], [], [$PATH:/sbin:/usr/sbin]) if test -z "$PARTED" ; then - with_storage_disk=no PARTED_FOUND=no else PARTED_FOUND=yes fi
- if test "$with_storage_disk" != "no"&& test "x$PKG_CONFIG" != "x" ; then - PKG_CHECK_MODULES(LIBPARTED, libparted>= $PARTED_REQUIRED, [], [PARTED_FOUND=no]) + if test "$PARTED_FOUND" = yes&& test "x$PKG_CONFIG" != "x" ; then
Just one small nit - almost everywhere else in configure.ac, the yes or no is quoted, but here you haven't quoted it. Operationally it makes no difference, it just looks "different" :-) At any rate, ACK.
On 01/31/2011 11:50 PM, Laine Stump wrote:
On 01/31/2011 05:10 PM, Eric Blake wrote:
* configure.ac (PARTED_FOUND): Issue configure error if --with-storage-disk=yes but no parted is found.
- if test "$with_storage_disk" != "no"&& test "x$PKG_CONFIG" != "x" ; then - PKG_CHECK_MODULES(LIBPARTED, libparted>= $PARTED_REQUIRED, [], [PARTED_FOUND=no]) + if test "$PARTED_FOUND" = yes&& test "x$PKG_CONFIG" != "x" ; then
Just one small nit - almost everywhere else in configure.ac, the yes or no is quoted, but here you haven't quoted it. Operationally it makes no difference, it just looks "different" :-)
At any rate, ACK.
Okay, I fixed the quoting consistency, and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Laine Stump -
Osier Yang