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

On 02/08/2011 03:30 AM, Osier Yang wrote: 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. 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. 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). And, since it involves syscalls, can we cache the answer in a local bool variable, rather than calling it twice? If you want, you can use virCommandNewArgList instead of virCommandNew/virCommandAddArgList. 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org