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