[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]