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

[Jim Meyering]

Richard W.M. Jones wrote:
> Subject: [PATCH 2/2] New APIs: Query the relationship between LVM objects.
>
> These calls allow you to query the relationship between
> LVM objects, for example, which PVs contain a VG, or which
> LVs are contained in a VG.

Nice.
A couple of minor nits, plus a probably-too-generous
serving of style suggestions ;-)

...
> diff --git a/daemon/lvm.c b/daemon/lvm.c
> index b100cd3..5c5846a 100644
> --- a/daemon/lvm.c
> +++ b/daemon/lvm.c
> @@ -512,3 +512,90 @@ do_vgrename (const char *volgroup, const char *newvolgroup)
>
>    return 0;
>  }
> +
> +static char *
> +get_lvm_field (const char *cmd, const char *field, const char *device)
> +{
> +  char *out;
> +  char *err;
> +  int r;
> +
> +  r = command (&out, &err,

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.

> +               "/sbin/lvm", cmd,

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)

> +               "--unbuffered", "--noheadings", "-o", field,
> +               device, NULL);
...
> +static char **
> +get_lvm_fields (const char *cmd, const char *field, const char *device)
> +{
> +  char *out;
> +  char *err;
> +  int r;
> +
> +  r = command (&out, &err,

Here, too.

> +               "/sbin/lvm", cmd,
> +               "--unbuffered", "--noheadings", "-o", field,
> +               device, NULL);
> +  if (r == -1) {
> +    reply_with_error ("%s: %s", device, err);
> +    free (out);
> +    free (err);
> +    return NULL;
> +  }
...
> diff --git a/regressions/test-lvm-mapping.pl b/regressions/test-lvm-mapping.pl
...
> +unlink "test.img";

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.

> +open FILE, ">test.img" or die "test.img: $!";
> +truncate FILE, 256*1024*1024 or die "test.img: truncate: $!";
> +close FILE;

You definitely want to detect write failure, here.
Otherwise, subsequent cascade failures would likely be harder to
diagnose.

> +my $g = Sys::Guestfs->new ();
> +
> +#$g->set_verbose (1);
> +#$g->set_trace (1);
> +
> +$g->add_drive ("test.img");
> +$g->launch ();
> +
> +# Create an arrangement of PVs, VGs and LVs.
> +$g->sfdiskM ("/dev/sda", [",127", "128,"]);
> +
> +$g->pvcreate ("/dev/sda1");
> +$g->pvcreate ("/dev/sda2");
> +$g->vgcreate ("VG", ["/dev/sda1", "/dev/sda2"]);
> +
> +$g->lvcreate ("LV1", "VG", 32);
> +$g->lvcreate ("LV2", "VG", 32);
> +$g->lvcreate ("LV3", "VG", 32);
> +
> +# Now let's get the arrangement.
> +my @pvs = $g->pvs ();
> +my @lvs = $g->lvs ();
> +
> +my %pvuuids;
> +foreach (@pvs) {

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.

> +    my $uuid = $g->pvuuid ($_);
> +    $pvuuids{$uuid} = $_;
> +}
> +my %lvuuids;
> +foreach (@lvs) {
> +    my $uuid = $g->lvuuid ($_);
> +    $lvuuids{$uuid} = $_;
> +}
> +
> +# In this case there is only one VG, called "VG", but in a real
> +# program you'd want to repeat these steps for each VG that you found.
> +my @pvuuids_in_VG = $g->vgpvuuids ("VG");
> +my @lvuuids_in_VG = $g->vglvuuids ("VG");
> +
> +my @pvs_in_VG;
> +foreach (@pvuuids_in_VG) {
> +    push @pvs_in_VG, $pvuuids{$_};
> +}
> + at pvs_in_VG = sort @pvs_in_VG;
> +
> +my @lvs_in_VG;
> +foreach (@lvuuids_in_VG) {
> +    push @lvs_in_VG, $lvuuids{$_};
> +}
> + at lvs_in_VG = sort @lvs_in_VG;
> +
> +die "unexpected set of PVs for volume group VG: [",
> +    join (", ", @pvs_in_VG), "]\n"
> +    unless @pvs_in_VG == 2 &&
> +    $pvs_in_VG[0] eq "/dev/vda1" && $pvs_in_VG[1] eq "/dev/vda2";

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;
> +
> +unlink "test.img";
> 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