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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110131/91ec78d4/attachment-0001.sig>