[libvirt] [PATCH v2] 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. # virsh vol-list --pool osier Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 # parted /dev/mapper/3600a0b80005adb0b0000ab2d4cae9254 rm p1 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 v2: - s/virIsDeviceMapperDevice/virIsDevMapperDevice/g - replace "virRun" with "virCommand" - sort the list of util functions in "libvirt_private.syms" e.g. # virsh vol-list --pool osier Name Path ----------------------------------------- 3600a0b80005ad1d7000093604cae912fp1 /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 # virsh vol-delete /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 Vol /dev/mapper/3600a0b80005ad1d7000093604cae912fp1 deleted # virsh vol-list --pool osier Name Path ----------------------------------------- --- configure.ac | 19 ++++++++++++++ libvirt.spec.in | 1 + src/Makefile.am | 8 +++--- src/libvirt_private.syms | 5 ++- src/storage/parthelper.c | 14 +--------- src/storage/storage_backend_disk.c | 48 +++++++++++++++++++++-------------- src/util/util.c | 14 ++++++++++ src/util/util.h | 2 + 8 files changed, 73 insertions(+), 38 deletions(-) diff --git a/configure.ac b/configure.ac index 8a4083f..9517bfc 100644 --- a/configure.ac +++ b/configure.ac @@ -1706,12 +1706,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]) @@ -1739,11 +1746,23 @@ if test "$with_storage_disk" = "yes" || 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..a8848bb 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 88e270c..63bd721 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -881,6 +881,7 @@ virGetUserID; virGetUserName; virHexToBin; virIndexToDiskName; +virIsDevMapperDevice; virKillProcess; virMacAddrCompare; virParseMacAddr; @@ -895,6 +896,8 @@ virSetCloseExec; virSetNonBlock; virSetUIDGID; virSkipSpaces; +virStrcpy; +virStrncpy; virStrToDouble; virStrToLong_i; virStrToLong_l; @@ -902,8 +905,6 @@ virStrToLong_ll; virStrToLong_ui; virStrToLong_ul; virStrToLong_ull; -virStrcpy; -virStrncpy; virTimestamp; virVasprintf; 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..05eb8c3 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,7 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, char *part_num = NULL; char *devpath = NULL; char *devname, *srcname; + virCommandPtr cmd = NULL; int rc = -1; if (virFileResolveLink(vol->target.path, &devpath) < 0) { @@ -660,38 +662,46 @@ 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 (!virIsDevMapperDevice(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 (!virIsDevMapperDevice(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; - } + 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 = virCommandNew(PARTED); + virCommandAddArgList(cmd, + 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 = virCommandNew(DMSETUP); + virCommandAddArgList(cmd, "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 ee04ca9..fc2e0c9 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; } + +bool +virIsDevMapperDevice(const char *devname) +{ + struct stat buf; + + if (devname && + !stat(devname, &buf) && + 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..280f86e 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); + #endif /* __VIR_UTIL_H__ */ -- 1.7.3.2
On 02/08/2011 03:30 AM, Osier Yang wrote:
@@ -1739,11 +1746,23 @@ if test "$with_storage_disk" = "yes" || 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
Logic is still not right here. You have two separate if clauses that both try to convert with_storage_disk=check into with_storage_disk=yes/no, but after the first block has run, the second block never knwos that with_storage_disk used to be check. It needs to be something like: 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 That is, if with_storage_disk is yes, then insist that both programs be found; if with_storage_disk is check, then set with_storage_disk exactly once based on the presence of both programs. So we'll need to see a v3.
@@ -895,6 +896,8 @@ virSetCloseExec; virSetNonBlock; virSetUIDGID; virSkipSpaces; +virStrcpy; +virStrncpy; virStrToDouble; virStrToLong_i; virStrToLong_l; @@ -902,8 +905,6 @@ virStrToLong_ll; virStrToLong_ui; virStrToLong_ul; virStrToLong_ull; -virStrcpy; -virStrncpy;
Hmm, you must have used emacs sorting while in LC_ALL=en_US.UTF-8 instead of LC_ALL=C. Personally, I leave LC_ALL undefined, LANG=en_US.UTF-8, and LC_COLLATE=C, to get byte-value sorting everywhere. I'm not sure if it makes sense to keep these hunks.
@@ -660,38 +662,46 @@ 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 (!virIsDevMapperDevice(devpath) && !STRPREFIX(devname, srcname)) {
Let's swap the order of the conditionals. STRPREFIX is potentially faster than virIsDevMapperDevice (since the latter calls stat() directly and who knows what other syscalls in dm_is_dm_major).
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 (!virIsDevMapperDevice(devpath)) {
And, since it involves syscalls, can we cache the answer in a local bool variable, rather than calling it twice?
+ } else { + cmd = virCommandNew(DMSETUP); + virCommandAddArgList(cmd, "remove", "--force", devpath, NULL);
If you want, you can use virCommandNewArgList instead of virCommandNew/virCommandAddArgList.
+bool +virIsDevMapperDevice(const char *devname) +{ + struct stat buf; + + if (devname && + !stat(devname, &buf) && + dm_is_dm_major(major(buf.st_rdev)))
major() is not required by POSIX; are we guaranteed that this will compile everywhere? Also, are we sure that st_rdev is defined on all file types, or should we add an extra condition that S_ISBLK(buf.st_mode)? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年02月09日 00:52, Eric Blake 写道:
On 02/08/2011 03:30 AM, Osier Yang wrote:
@@ -1739,11 +1746,23 @@ if test "$with_storage_disk" = "yes" || 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
Logic is still not right here. You have two separate if clauses that both try to convert with_storage_disk=check into with_storage_disk=yes/no, but after the first block has run, the second block never knwos that with_storage_disk used to be check.
It needs to be something like:
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
That is, if with_storage_disk is yes, then insist that both programs be found; if with_storage_disk is check, then set with_storage_disk exactly once based on the presence of both programs.
Oops, yes, didn't pay much attention here. :-)
So we'll need to see a v3.
@@ -895,6 +896,8 @@ virSetCloseExec; virSetNonBlock; virSetUIDGID; virSkipSpaces; +virStrcpy; +virStrncpy; virStrToDouble; virStrToLong_i; virStrToLong_l; @@ -902,8 +905,6 @@ virStrToLong_ll; virStrToLong_ui; virStrToLong_ul; virStrToLong_ull; -virStrcpy; -virStrncpy;
Hmm, you must have used emacs sorting while in LC_ALL=en_US.UTF-8 instead of LC_ALL=C. Personally, I leave LC_ALL undefined, LANG=en_US.UTF-8, and LC_COLLATE=C, to get byte-value sorting everywhere. I'm not sure if it makes sense to keep these hunks.
@@ -660,38 +662,46 @@ 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 (!virIsDevMapperDevice(devpath)&& !STRPREFIX(devname, srcname)) {
Let's swap the order of the conditionals. STRPREFIX is potentially faster than virIsDevMapperDevice (since the latter calls stat() directly and who knows what other syscalls in dm_is_dm_major).
yes, carefully reviewing, will update.
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 (!virIsDevMapperDevice(devpath)) {
And, since it involves syscalls, can we cache the answer in a local bool variable, rather than calling it twice?
will update
+ } else { + cmd = virCommandNew(DMSETUP); + virCommandAddArgList(cmd, "remove", "--force", devpath, NULL);
If you want, you can use virCommandNewArgList instead of virCommandNew/virCommandAddArgList.
It's more compact, so, okay.
+bool +virIsDevMapperDevice(const char *devname) +{ + struct stat buf; + + if (devname&& + !stat(devname,&buf)&& + dm_is_dm_major(major(buf.st_rdev)))
major() is not required by POSIX; are we guaranteed that this will compile everywhere? Also, are we sure that st_rdev is defined on all file types, or should we add an extra condition that S_ISBLK(buf.st_mode)?
On 02/08/2011 03:30 AM, Osier Yang wrote:
+bool +virIsDevMapperDevice(const char *devname) +{ + struct stat buf; + + if (devname && + !stat(devname, &buf) &&
One other thing - the check for devname being non-NULL is not necessary...
+bool virIsDevMapperDevice(const char *devname);
if you add ATTRIBUTE_NONNULL(1) to this declaration. Given that the only use of this new function to date doesn't pass a NULL pointer, we might as well make that part of the API. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年02月09日 01:12, Eric Blake 写道:
On 02/08/2011 03:30 AM, Osier Yang wrote:
+bool +virIsDevMapperDevice(const char *devname) +{ + struct stat buf; + + if (devname&& + !stat(devname,&buf)&&
One other thing - the check for devname being non-NULL is not necessary...
+bool virIsDevMapperDevice(const char *devname);
if you add ATTRIBUTE_NONNULL(1) to this declaration. Given that the only use of this new function to date doesn't pass a NULL pointer, we might as well make that part of the API.
Yeah, it's more reasonable to add ATTRIBUTE_NONNULL to this internal function, but do we need to make change on the API? I'm not sure if it will introduce some problem. Regards Osier
On 02/08/2011 08:02 PM, Osier Yang wrote:
于 2011年02月09日 01:12, Eric Blake 写道:
On 02/08/2011 03:30 AM, Osier Yang wrote:
+bool +virIsDevMapperDevice(const char *devname) +{ + struct stat buf; + + if (devname&& + !stat(devname,&buf)&&
One other thing - the check for devname being non-NULL is not necessary...
+bool virIsDevMapperDevice(const char *devname);
if you add ATTRIBUTE_NONNULL(1) to this declaration. Given that the only use of this new function to date doesn't pass a NULL pointer, we might as well make that part of the API.
Yeah, it's more reasonable to add ATTRIBUTE_NONNULL to this internal function, but do we need to make change on the API? I'm not sure if it will introduce some problem.
Adding ATTRIBUTE_NONNULL _is_ the API change I was referring to - it simplifies the implementation, and puts a contract on the caller to not pass NULL. We can always remove it later (and re-instate the explicit null check in the implementation) if we find a reason to, since the function is internal. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Osier Yang