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

[Osier Yang]

于 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)?
>