Libguestfs - Jul 2018 - Re: [PATCH 2/3] New API: lvm_scan, deprecate vgscan (RHBZ#1602353).

[This email is either empty or too large to be displayed at this time]

This is not added to guests-all-good since it cannot be used in
automated tests -- a password must be supplied.
---
 .gitignore                                |  1 +
 test-data/phony-guests/Makefile.am        |  8 ++++
 test-data/phony-guests/guests.xml.in      | 49 +++++++++++++++--------
 test-data/phony-guests/make-fedora-img.pl | 29 ++++++++++++++
 4 files changed, 71 insertions(+), 16 deletions(-)

diff --git a/.gitignore b/.gitignore
index d855459f3..bb4f45e0a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -650,6 +650,7 @@ Makefile.in
 /test-data/phony-guests/debian.img
 /test-data/phony-guests/fedora.img
 /test-data/phony-guests/fedora-btrfs.img
+/test-data/phony-guests/fedora-luks.img
 /test-data/phony-guests/fedora-md1.img
 /test-data/phony-guests/fedora-md2.img
 /test-data/phony-guests/fedora-name.db
diff --git a/test-data/phony-guests/Makefile.am
b/test-data/phony-guests/Makefile.am
index 4e978f654..6a0d23e04 100644
--- a/test-data/phony-guests/Makefile.am
+++ b/test-data/phony-guests/Makefile.am
@@ -51,6 +51,7 @@ disk_images = \
 	fedora-md1.img \
 	fedora-md2.img \
 	fedora-btrfs.img \
+	fedora-luks.img \
 	ubuntu.img \
 	archlinux.img \
 	coreos.img \
@@ -100,6 +101,13 @@ fedora-btrfs.img: make-fedora-img.pl \
 		fedora-packages.db
 	SRCDIR=$(srcdir) LAYOUT=btrfs $(top_builddir)/run --test ./$<
 
+# Make a (dummy) Fedora image with LVM encrypted with LUKS.
+fedora-luks.img: make-fedora-img.pl \
+		fedora-journal.tar.xz \
+		fedora-name.db \
+		fedora-packages.db
+	SRCDIR=$(srcdir) LAYOUT=lvm-luks $(top_builddir)/run --test ./$<
+
 # Make a (dummy) Debian image.
 debian.img: make-debian-img.sh
 	SRCDIR=$(srcdir) $(top_builddir)/run --test ./$<
diff --git a/test-data/phony-guests/guests.xml.in
b/test-data/phony-guests/guests.xml.in
index 9c7c989dc..4139d04f6 100644
--- a/test-data/phony-guests/guests.xml.in
+++ b/test-data/phony-guests/guests.xml.in
@@ -151,22 +151,6 @@
     </devices>
   </domain>
 
-  <domain type='test'>
-    <name>fedora-btrfs</name>
-    <memory>1048576</memory>
-    <os>
-      <type>hvm</type>
-      <boot dev='hd'/>
-    </os>
-    <devices>
-      <disk type='file' device='disk'>
-        <driver name='qemu' type='raw'/>
-        <source file='@abs_builddir@/fedora-btrfs.img'/>
-        <target dev='vda' bus='virtio'/>
-      </disk>
-    </devices>
-  </domain>
-
   <domain type='test'>
     <name>fedora</name>
     <memory>1048576</memory>
@@ -183,6 +167,39 @@
     </devices>
   </domain>
 
+  <domain type='test'>
+    <name>fedora-btrfs</name>
+    <memory>1048576</memory>
+    <os>
+      <type>hvm</type>
+      <boot dev='hd'/>
+    </os>
+    <devices>
+      <disk type='file' device='disk'>
+        <driver name='qemu' type='raw'/>
+        <source file='@abs_builddir@/fedora-btrfs.img'/>
+        <target dev='vda' bus='virtio'/>
+      </disk>
+    </devices>
+  </domain>
+
+  <!-- LUKS password is 'FEDORA' -->
+  <domain type='test'>
+    <name>fedora-luks</name>
+    <memory>1048576</memory>
+    <os>
+      <type>hvm</type>
+      <boot dev='hd'/>
+    </os>
+    <devices>
+      <disk type='file' device='disk'>
+        <driver name='qemu' type='raw'/>
+        <source file='@abs_builddir@/fedora-luks.img'/>
+        <target dev='vda' bus='virtio'/>
+      </disk>
+    </devices>
+  </domain>
+
   <domain type='test'>
     <name>fedora-md1</name>
     <memory>1048576</memory>
diff --git a/test-data/phony-guests/make-fedora-img.pl
b/test-data/phony-guests/make-fedora-img.pl
index cd1c4a48c..c665f0d19 100755
--- a/test-data/phony-guests/make-fedora-img.pl
+++ b/test-data/phony-guests/make-fedora-img.pl
@@ -153,6 +153,35 @@ EOF
   $g->mount ('btrfsvol:/dev/sda2/root', '/');
 }
 
+elsif ($ENV{LAYOUT} eq 'lvm-luks') {
+  push (@images, "fedora-luks.img-t");
+
+  open (my $fstab, '>', "fedora.fstab") or die;
+  print $fstab <<EOF;
+LABEL=BOOT /boot ext2 default 0 0
+LABEL=ROOT / ext2 default 0 0
+EOF
+  close ($fstab) or die;
+
+  $bootdev = '/dev/sda1';
+
+  $g->disk_create ("fedora-luks.img-t", "raw",
$IMAGE_SIZE);
+
+  $g->add_drive ("fedora-luks.img-t", format =>
"raw");
+  $g->launch ();
+
+  $g->part_init ('/dev/sda', 'mbr');
+  foreach my $p (@PARTITIONS) {
+    $g->part_add('/dev/sda', @$p);
+  }
+
+  # Put LUKS on the second partition.
+  $g->luks_format ('/dev/sda2', 'FEDORA', 0);
+  $g->luks_open ('/dev/sda2', 'FEDORA', 'luks');
+
+  init_lvm_root ('/dev/mapper/luks');
+}
+
 else {
   print STDERR "$0: Unknown LAYOUT: ",$ENV{LAYOUT},"\n";
   exit 1;
-- 
2.18.0

The old vgscan API literally ran vgscan.  When we switched to using
lvmetad (in commit dd162d2cd56a2ecf4bcd40a7f463940eaac875b8) this
stopped working because lvmetad now ignores plain vgscan commands
without the --cache option.

We documented that vgscan would rescan PVs, VGs and LVs, but without
activating them.

I have introduced a new API (lvm_scan) which scans or rescans PVs, VGs
and LVs.  It has an optional activate parameter allowing activation of
any new LVs that are found.

With lvmetad this nicely maps to the single command:

 pvscan --cache [--activate ay]
---
 common/options/decrypt.c             |  4 +--
 daemon/lvm.c                         | 22 ++++++++++++++--
 format/format.c                      |  2 +-
 generator/actions_core.ml            | 38 ++++++++++++++++++----------
 generator/actions_core_deprecated.ml | 13 ++++++++++
 generator/proc_nr.ml                 |  1 +
 lib/MAX_PROC_NR                      |  2 +-
 7 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/common/options/decrypt.c b/common/options/decrypt.c
index d6e041db6..b9113073c 100644
--- a/common/options/decrypt.c
+++ b/common/options/decrypt.c
@@ -94,9 +94,7 @@ inspect_do_decrypt (guestfs_h *g)
   }
 
   if (need_rescan) {
-    if (guestfs_vgscan (g) == -1)
-      exit (EXIT_FAILURE);
-    if (guestfs_vg_activate_all (g, 1) == -1)
+    if (guestfs_lvm_scan (g, 1) == -1)
       exit (EXIT_FAILURE);
   }
 }
diff --git a/daemon/lvm.c b/daemon/lvm.c
index 877bc5f6c..4fe2c24e4 100644
--- a/daemon/lvm.c
+++ b/daemon/lvm.c
@@ -32,6 +32,8 @@
 #include "actions.h"
 #include "optgroups.h"
 
+#define MAX_ARGS 64
+
 int
 optgroup_lvm2_available (void)
 {
@@ -637,12 +639,28 @@ do_vglvuuids (const char *vgname)
 
 int
 do_vgscan (void)
+{
+  return do_lvm_scan (0);
+}
+
+int
+do_lvm_scan (int activate)
 {
   CLEANUP_FREE char *err = NULL;
   int r;
+  const char *argv[MAX_ARGS];
+  size_t i = 0;
 
-  r = command (NULL, &err,
-               "lvm", "vgscan", NULL);
+  ADD_ARG (argv, i, "lvm");
+  ADD_ARG (argv, i, "pvscan");
+  ADD_ARG (argv, i, "--cache");
+  if (activate) {
+    ADD_ARG (argv, i, "--activate");
+    ADD_ARG (argv, i, "ay");
+  }
+  ADD_ARG (argv, i, NULL);
+
+  r = commandv (NULL, &err, (const char * const *) argv);
   if (r == -1) {
     reply_with_error ("%s", err);
     return -1;
diff --git a/format/format.c b/format/format.c
index 57f74669c..5453fc7e1 100644
--- a/format/format.c
+++ b/format/format.c
@@ -466,7 +466,7 @@ do_rescan (char **devices)
       errors++;
   }
 
-  if (guestfs_vgscan (g) == -1)
+  if (guestfs_lvm_scan (g, 1) == -1)
     errors++;
 
   guestfs_pop_error_handler (g);
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index 5fb49a14d..f237a3c8b 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -5245,18 +5245,6 @@ If blocks are already zero, then this command avoids
writing
 zeroes.  This prevents the underlying device from becoming non-sparse
 or growing unnecessarily." };
 
-  { defaults with
-    name = "vgscan"; added = (1, 3, 2);
-    style = RErr, [], [];
-    tests = [
-      InitEmpty, Always, TestRun (
-        [["vgscan"]]), []
-    ];
-    shortdesc = "rescan for LVM physical volumes, volume groups and
logical volumes";
-    longdesc = "\
-This rescans all block devices and rebuilds the list of LVM
-physical volumes, volume groups and logical volumes." };
-
   { defaults with
     name = "part_del"; added = (1, 3, 2);
     style = RErr, [String (Device, "device"); Int
"partnum"], [];
@@ -5660,8 +5648,8 @@ Reads and writes to this block device are decrypted from
and
 encrypted to the underlying C<device> respectively.
 
 If this block device contains LVM volume groups, then
-calling C<guestfs_vgscan> followed by C<guestfs_vg_activate_all>
-will make them visible.
+calling C<guestfs_lvm_scan> with the C<activate>
+parameter C<true> will make them visible.
 
 Use C<guestfs_list_dm_devices> to list all device mapper
 devices." };
@@ -9718,4 +9706,26 @@ before the partition." };
 This expands a f2fs filesystem to match the size of the underlying
 device." };
 
+  { defaults with
+    name = "lvm_scan"; added = (1, 39, 8);
+    style = RErr, [Bool "activate"], [];
+    tests = [
+      InitEmpty, Always, TestRun (
+        [["lvm_scan"; "true"]]), []
+    ];
+    shortdesc = "scan for LVM physical volumes, volume groups and logical
volumes";
+    longdesc = "\
+This scans all block devices and rebuilds the list of LVM
+physical volumes, volume groups and logical volumes.
+
+If the C<activate> parameter is C<true> then newly found
+volume groups and logical volumes are activated, meaning
+the LV F</dev/VG/LV> devices become visible.
+
+When a libguestfs handle is launched it scans for existing
+devices, so you do not normally need to use this API.  However
+it is useful when you have added a new device or deleted an
+existing device (such as when the C<guestfs_luks_open> API
+is used)." };
+
 ]
diff --git a/generator/actions_core_deprecated.ml
b/generator/actions_core_deprecated.ml
index 9a58b1df3..fafb9adad 100644
--- a/generator/actions_core_deprecated.ml
+++ b/generator/actions_core_deprecated.ml
@@ -834,4 +834,17 @@ is not large enough." };
 This command is the same as C<guestfs_ntfsresize> except that it
 allows you to specify the new size (in bytes) explicitly." };
 
+  { defaults with
+    name = "vgscan"; added = (1, 3, 2);
+    style = RErr, [], [];
+    deprecated_by = Replaced_by "lvm_scan";
+    tests = [
+      InitEmpty, Always, TestRun (
+        [["vgscan"]]), []
+    ];
+    shortdesc = "rescan for LVM physical volumes, volume groups and
logical volumes";
+    longdesc = "\
+This rescans all block devices and rebuilds the list of LVM
+physical volumes, volume groups and logical volumes." };
+
 ]
diff --git a/generator/proc_nr.ml b/generator/proc_nr.ml
index ca73aa361..2adf8a32f 100644
--- a/generator/proc_nr.ml
+++ b/generator/proc_nr.ml
@@ -513,6 +513,7 @@ let proc_nr = [
 503, "part_set_gpt_attributes";
 504, "part_get_gpt_attributes";
 505, "f2fs_expand";
+506, "lvm_scan";
 ]
 
 (* End of list.  If adding a new entry, add it at the end of the list
diff --git a/lib/MAX_PROC_NR b/lib/MAX_PROC_NR
index f573e999a..80e3e6eab 100644
--- a/lib/MAX_PROC_NR
+++ b/lib/MAX_PROC_NR
@@ -1 +1 @@
-505
+506
-- 
2.18.0

---
 .gitignore                               |  1 +
 configure.ac                             |  2 +
 inspector/Makefile.am                    |  7 +++-
 inspector/expected-fedora-luks.img.xml   | 52 ++++++++++++++++++++++++
 inspector/test-virt-inspector-luks.sh.in | 42 +++++++++++++++++++
 5 files changed, 102 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index bb4f45e0a..6c6ca7ac6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -352,6 +352,7 @@ Makefile.in
 /inspector/actual-*.xml
 /inspector/stamp-virt-inspector.pod
 /inspector/test-virt-inspector.sh
+/inspector/test-virt-inspector-luks.sh
 /inspector/test-xmllint.sh
 /inspector/virt-inspector
 /inspector/virt-inspector.1
diff --git a/configure.ac b/configure.ac
index b2b64bdd9..baed62f92 100644
--- a/configure.ac
+++ b/configure.ac
@@ -192,6 +192,8 @@ AC_CONFIG_FILES([inspector/test-xmllint.sh],
                 [chmod +x,-w inspector/test-xmllint.sh])
 AC_CONFIG_FILES([inspector/test-virt-inspector.sh],
                 [chmod +x,-w inspector/test-virt-inspector.sh])
+AC_CONFIG_FILES([inspector/test-virt-inspector-luks.sh],
+                [chmod +x,-w inspector/test-virt-inspector-luks.sh])
 AC_CONFIG_FILES([installcheck.sh],
                 [chmod +x,-w installcheck.sh])
 AC_CONFIG_FILES([ocaml-dep.sh],
diff --git a/inspector/Makefile.am b/inspector/Makefile.am
index 00d894199..82ff4be77 100644
--- a/inspector/Makefile.am
+++ b/inspector/Makefile.am
@@ -27,11 +27,13 @@ example_xml = \
 EXTRA_DIST = \
 	expected-debian.img.xml \
 	expected-fedora.img.xml \
+	expected-fedora-luks.img.xml \
 	expected-ubuntu.img.xml \
 	expected-archlinux.img.xml \
 	expected-coreos.img.xml \
 	expected-windows.img.xml \
 	test-virt-inspector.sh \
+	test-virt-inspector-luks.sh \
 	test-virt-inspector-docs.sh \
 	test-xmllint.sh.in \
 	virt-inspector.pod
@@ -92,14 +94,15 @@ TESTS = \
 
 if ENABLE_APPLIANCE
 TESTS += \
-	test-virt-inspector.sh
+	test-virt-inspector.sh \
+	test-virt-inspector-luks.sh
 endif ENABLE_APPLIANCE
 if HAVE_XMLLINT
 TESTS += test-xmllint.sh
 endif
 
 check-valgrind:
-	$(MAKE) TESTS="test-virt-inspector.sh" VG="@VG@" check
+	$(MAKE) TESTS="test-virt-inspector.sh test-virt-inspector-luks.sh"
VG="@VG@" check
 
 check-valgrind-local-guests:
 	for g in $(GUESTS); do \
diff --git a/inspector/expected-fedora-luks.img.xml
b/inspector/expected-fedora-luks.img.xml
new file mode 100644
index 000000000..df6060a73
--- /dev/null
+++ b/inspector/expected-fedora-luks.img.xml
@@ -0,0 +1,52 @@
+<?xml version="1.0"?>
+<operatingsystems>
+  <operatingsystem>
+    <root>/dev/VG/Root</root>
+    <name>linux</name>
+    <arch>x86_64</arch>
+    <distro>fedora</distro>
+    <product_name>Fedora release 14 (Phony)</product_name>
+    <major_version>14</major_version>
+    <minor_version>0</minor_version>
+    <package_format>rpm</package_format>
+    <package_management>yum</package_management>
+    <hostname>fedora.invalid</hostname>
+    <osinfo>fedora14</osinfo>
+    <mountpoints>
+      <mountpoint dev="/dev/VG/Root">/</mountpoint>
+      <mountpoint dev="/dev/sda1">/boot</mountpoint>
+    </mountpoints>
+    <filesystems>
+      <filesystem dev="/dev/VG/Root">
+        <type>ext2</type>
+        <label>ROOT</label>
+        <uuid>01234567-0123-0123-0123-012345678902</uuid>
+      </filesystem>
+      <filesystem dev="/dev/sda1">
+        <type>ext2</type>
+        <label>BOOT</label>
+        <uuid>01234567-0123-0123-0123-012345678901</uuid>
+      </filesystem>
+    </filesystems>
+    <applications>
+      <application>
+        <name>test1</name>
+        <version>1.0</version>
+        <release>1.fc14</release>
+        <arch>x86_64</arch>
+      </application>
+      <application>
+        <name>test2</name>
+        <version>2.0</version>
+        <release>2.fc14</release>
+        <arch>x86_64</arch>
+      </application>
+      <application>
+        <name>test3</name>
+        <version>3.0</version>
+        <release>3.fc14</release>
+        <arch>x86_64</arch>
+      </application>
+    </applications>
+  </operatingsystem>
+</operatingsystems>
diff --git a/inspector/test-virt-inspector-luks.sh.in
b/inspector/test-virt-inspector-luks.sh.in
new file mode 100755
index 000000000..8579846f6
--- /dev/null
+++ b/inspector/test-virt-inspector-luks.sh.in
@@ -0,0 +1,42 @@
+#!/bin/bash -
+# libguestfs virt-inspector test script
+# Copyright (C) 2012-2018 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test that virt-inspector can work on encrypted images when the
+# right password is supplied.
+#
+# Regression test for https://bugzilla.redhat.com/show_bug.cgi?id=1602353
+
+set -e
+set -x
+
+$TEST_FUNCTIONS
+skip_if_skipped
+
+f=../test-data/phony-guests/fedora-luks.img
+
+# Ignore zero-sized file.
+if [ -s "$f" ]; then
+    b=$(basename "$f")
+    echo FEDORA |
+    $VG virt-inspector --keys-from-stdin --format=raw -a "$f" >
"actual-$b.xml"
+    # Check the generated output validate the schema.
+    @XMLLINT@ --noout --relaxng "$srcdir/virt-inspector.rng"
"actual-$b.xml"
+    # This 'diff' command will fail (because of -e option) if there
+    # are any differences.
+    diff -ur $diff_ignore "expected-$b.xml" "actual-$b.xml"
+fi
-- 
2.18.0

On Wednesday, 18 July 2018 15:37:24 CEST Richard W.M. Jones
wrote:
> The old vgscan API literally ran vgscan.  When we switched to using
> lvmetad (in commit dd162d2cd56a2ecf4bcd40a7f463940eaac875b8) this
> stopped working because lvmetad now ignores plain vgscan commands
> without the --cache option.
> 
> We documented that vgscan would rescan PVs, VGs and LVs, but without
> activating them.
> 
> I have introduced a new API (lvm_scan) which scans or rescans PVs, VGs
> and LVs.  It has an optional activate parameter allowing activation of
> any new LVs that are found.
> 
> With lvmetad this nicely maps to the single command:
> 
>  pvscan --cache [--activate ay]
> ---
lvm_scan seems basically vgscan + activate -- wouldn't be better to just
add the "activate" optarg to vgscan? After all, if vgscan is
deprecated,
and users should migrate to lvm_scan, they can just add a new optarg.

-- 
Pino Toscano

On Wednesday, 18 July 2018 15:37:24 CEST Richard W.M. Jones
wrote:
> The old vgscan API literally ran vgscan.  When we switched to using
> lvmetad (in commit dd162d2cd56a2ecf4bcd40a7f463940eaac875b8) this
> stopped working because lvmetad now ignores plain vgscan commands
> without the --cache option.
Yes and no, see below.
> +int
> +do_lvm_scan (int activate)
>  {
>    CLEANUP_FREE char *err = NULL;
>    int r;
> +  const char *argv[MAX_ARGS];
> +  size_t i = 0;
>  
> -  r = command (NULL, &err,
> -               "lvm", "vgscan", NULL);
> +  ADD_ARG (argv, i, "lvm");
> +  ADD_ARG (argv, i, "pvscan");
> +  ADD_ARG (argv, i, "--cache");
--cache is basically the difference, and why it should work. This was
already touched in the past, see commit
2c4f154b91215e775c77920b291ac081e93542fd later amended by commit
19b0ae6c6502c4833c73e1b1bfa47b43a40f21e9. The second commit (i.e.
remove --cache) was done because older distros at that time had an old
lvm2 versions without lvmetad (or configured to not use lvmetad, even
if running). The recent commit dd162d2cd56a2ecf4bcd40a7f463940eaac875b8
changed the way lvmetad is run, although I do not remember if it is
safe to assume that lvmetad is both available and "usable" in all the
distros we support.

All in all, my suggestion would be the following:
1) since the behaviour of the vgscan API is changed, then revert
   commit 19b0ae6c6502c4833c73e1b1bfa47b43a40f21e9, so its behaviour
   will be as the patch does
2) add the new APIs in a second commit
  
This way the fix for vgscan could be backported to 1.38.x, if it is
safe enough, leaving the new API for 1.39+.

Also, just (1) should be enough to 	fix rhbz#1602353, since the decrypt
code already does vgscan + vg_activate_all, which is what lvm_scan
does.

-- 
Pino Toscano