8:59 a.m.
Currently I found it it's hard to determine the relationship between
LVM objects, by which I mean "what PVs contain a VG?", or "what LVs
are contained in a VG?"
This simple API exposes that to callers.
{lv,vg,pv}uuid:
Return the UUID of an LVM object. You can already get this
using (eg.) lvs_full, but this is a lot less faffing around.
vg{lv,pv}uuids:
Return the LVs belonging to a VG, or the PVs containing a VG.
It returns them as UUIDs, so if you build up a map using the
previous calls, then you can map them back to names, or keep
them as UUIDs as required.
There's an example Perl program included which demonstrates how to do
this.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
9 a.m.
New subject: [PATCH 1/2] daemon: Add a trim utility function.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
10:04 a.m.
New subject: [PATCH 1/2] daemon: Add a trim utility function.
Richard W.M. Jones wrote:
Subject: [PATCH 1/2] daemon: Add a trim utility function.
This function trims the whitespace from around a string. It
does this in-place, so it can be called for malloc'd strings.
...
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
...
+/* Skip leading and trailing whitespace, updating the original
string
+ * in-place.
+ */
+void
+trim (char *str)
+{
+ size_t len = strlen (str);
+
+ while (len > 0 && c_isspace (str[len-1])) {
+ str[len-1] = '\0';
+ len--;
+ }
You might want to avoid the writes when there are two or
more trailing whitespace bytes. It's enough just to write
the final (leftmost) one.
+
+ char *p = str;
This can be "const".
+ while (*p && c_isspace (*p)) {
+ p++;
+ len--;
+ }
You might want to replace the loop with
size_t n_leading_sp = strspn (p, " \t\n\v\f\r")
p += n_leading_sp;
len -= n_leading_sp;
+ memmove (str, p, len+1);
+}
It probably doesn't matter, since this function
is unlikely to be applied in an inner loop or to very long
strings with lots of leading and trailing blanks, but...
You could write this function so that it reads the input string
only once, rather than 2 or 3 times.
10:22 a.m.
New subject: [PATCH 1/2] daemon: Add a trim utility function.
On Thu, Mar 18, 2010 at 04:04:32PM +0100, Jim Meyering wrote:
It probably doesn't matter, since this function
is unlikely to be applied in an inner loop or to very long
strings with lots of leading and trailing blanks, but...
You could write this function so that it reads the input string
only once, rather than 2 or 3 times.
Thanks -- I've made some adjustments to this function. Speedwise it's
not hugely important when you consider that we immediately dispatch
the strings over an RPC-encoded loopback network socket.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
9 a.m.
New subject: [PATCH 2/2] New APIs: Query the relationship between LVM objects.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/
9:36 a.m.
New subject: [PATCH 2/2] New APIs: Query the relationship between LVM objects.
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{$_};
+}
+@pvs_in_VG = sort @pvs_in_VG;
+
+my @lvs_in_VG;
+foreach (@lvuuids_in_VG) {
+ push @lvs_in_VG, $lvuuids{$_};
+}
+@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
9:42 a.m.
New subject: [PATCH 2/2] New APIs: Query the relationship between LVM objects.
On Thu, Mar 18, 2010 at 03:36:14PM +0100, Jim Meyering wrote:
> + "/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)
These are a kind of "note to self" to fix path handling in the daemon.
The path should be set to include /sbin and /usr/sbin and then we can
remove all hard-coded references like this in one go.
Thanks for the review. I'll make the other changes that you
suggested.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
5287
days inactive
5287
days old
6 comments
2 participants
participants (2)
-
Jim Meyering -
Richard W.M. Jones