Re: [Libguestfs] [PATCH 2/2] New APIs: Query the relationship between LVM objects.

Richard W.M. Jones wrote: Nice. A couple of minor nits, plus a probably-too-generous serving of style suggestions ;-) ... You can save two lines by combining declaration and initialization: int r = command (&out, &err, Unless you have one of those monster 2560x1600 monitors, vertical space is rather limited. You probably have a fine reason to do it, but seeing hard-coded absolute tool names like that makes me cringe. (yeah, I see it already appears many times in lvm.c, but still) ... Here, too. > + if (r == -1) { > + reply_with_error ("%s: %s", device, err); > + free (out); > + free (err); > + return NULL; > + } ... ... Even if it's just a test, it's nice to detect failure: unlink "test.img" or die "failed to unlink test.img: $!\n"; With all of those literal "test.img" strings, it'd be nice to factor it out. You definitely want to detect write failure, here. Otherwise, subsequent cascade failures would likely be harder to diagnose. IMHO, it's best to avoid using $_ most of the time. I would write the above as foreach my $pv (@pvs) { and then use $pv in place of $_. Otherwise, it's too easy to forget to declare $_ local, and you end up with very hard to diagnose problems when some callee updates the global $_. Attaching a name to the index variable can help with readability, too, at least in less trivial loops. I find the "die ... unless cond" idiom to be less readable than e.g., cond or die The latter starts with the conditional and makes it clearer (to me at least) that the "die" part is conditional. But when I see a line that starts with "die ...", I tend to think it's going to be unconditional, and only when I read the rest do I find the "unless" part and realize that, no ... > +die "unexpected set of LVs for volume group VG: [", > + join (", ", @lvs_in_VG), "]\n" > + unless @lvs_in_VG == 3 && > + $lvs_in_VG[0] eq "/dev/VG/LV1" && > + $lvs_in_VG[1] eq "/dev/VG/LV2" && > + $lvs_in_VG[2] eq "/dev/VG/LV3"; > + > +undef $g; > + > diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR > index 7b47338..f414671 100644 > --- a/src/MAX_PROC_NR > +++ b/src/MAX_PROC_NR > @@ -1 +1 @@ > -221 > +226 > diff --git a/src/generator.ml b/src/generator.ml > index 83f307b..fdd228e 100755 > --- a/src/generator.ml > +++ b/src/generator.ml > @@ -4290,6 +4290,48 @@ contained in a Linux initrd or initramfs image: > > See also C<guestfs_initrd_list>."); > > + ("pvuuid", (RString "uuid", [Device "device"]), 222, [], > + [], > + "get the UUID of a physical volume", > + "\ > +This command returns the UUID of the LVM PV C<device>."); > + > + ("vguuid", (RString "uuid", [String "vgname"]), 223, [], > + [], > + "get the UUID of a volume group", > + "\ > +This command returns the UUID of the LVM VG named C<vgname>."); > + > + ("lvuuid", (RString "uuid", [Device "device"]), 224, [], > + [], > + "get the UUID of a logical volume", > + "\ > +This command returns the UUID of the LVM LV C<device>."); > + > + ("vgpvuuids", (RStringList "uuids", [String "vgname"]), 225, [], > + [], > + "get the PV UUIDs containing the volume group", > + "\ > +Given a VG called C<vgname>, this returns the UUIDs of all > +the physical volumes that this volume group resides on. > + > +You can use this along with C<guestfs_pvs> and C<guestfs_pvuuid> > +calls to associate physical volumes and volume groups. > + > +See also C<guestfs_vglvuuids>."); > + > + ("vglvuuids", (RStringList "uuids", [String "vgname"]), 226, [], > + [], > + "get the LV UUIDs of all LVs in the volume group", > + "\ > +Given a VG called C<vgname>, this returns the UUIDs of all > +the logical volumes created in this volume group. > + > +You can use this along with C<guestfs_lvs> and C<guestfs_lvuuid> > +calls to associate logical volumes and volume groups. > + > +See also C<guestfs_vgpvuuids>."); > + > ] > > let all_functions = non_daemon_functions @ daemon_functions