Libguestfs - Jul 2017 - Re: check-syntax FAILED (was: Re: [PATCH 2/2] tests: lvm: Make the lvm_set_filter test easier to understand.)

Simplify how we handle lvm.conf.

LVM is fine with a completely empty configuration file (meaning "all
defaults"), so start with one instead of copying the system
configuration file.

Also this means we can very easily implement lvm_set_filter
functionality without using Augeas, since we no longer have to worry
about existing filters being present.
---
 appliance/Makefile.am          |   4 +-
 appliance/guestfs_lvm_conf.aug |  74 -----------------
 daemon/daemon.h                |   2 +-
 daemon/guestfsd.c              |   2 +-
 daemon/lvm-filter.c            | 178 +++++++++++++----------------------------
 5 files changed, 59 insertions(+), 201 deletions(-)

diff --git a/appliance/Makefile.am b/appliance/Makefile.am
index 05b9d42e9..42896b9a7 100644
--- a/appliance/Makefile.am
+++ b/appliance/Makefile.am
@@ -32,7 +32,6 @@ include $(top_srcdir)/subdir-rules.mk
 EXTRA_DIST = \
 	99-guestfs-serial.rules \
 	excludefiles.in \
-	guestfs_lvm_conf.aug \
 	guestfs_shadow.aug \
 	hostfiles.in \
 	init \
@@ -87,12 +86,11 @@ packagelist: packagelist.in Makefile
 	cmp -s $@ $@-t || mv $@-t $@
 	rm -f $@-t
 
-supermin.d/daemon.tar.gz: ../daemon/guestfsd guestfs_lvm_conf.aug
guestfs_shadow.aug
+supermin.d/daemon.tar.gz: ../daemon/guestfsd guestfs_shadow.aug
 	rm -f $@ $@-t
 	rm -rf tmp-d
 	mkdir -p tmp-d$(DAEMON_SUPERMIN_DIR) tmp-d/etc tmp-d/usr/share/guestfs
 	ln ../daemon/guestfsd tmp-d$(DAEMON_SUPERMIN_DIR)/guestfsd
-	ln $(srcdir)/guestfs_lvm_conf.aug tmp-d/usr/share/guestfs/guestfs_lvm_conf.aug
 	ln $(srcdir)/guestfs_shadow.aug tmp-d/usr/share/guestfs/guestfs_shadow.aug
 	( cd tmp-d && tar zcf - * ) > $@-t
 	rm -r tmp-d
diff --git a/appliance/guestfs_lvm_conf.aug b/appliance/guestfs_lvm_conf.aug
deleted file mode 100644
index ffa5b01f7..000000000
--- a/appliance/guestfs_lvm_conf.aug
+++ /dev/null
@@ -1,74 +0,0 @@
-(*
-Module: LVM
-  Parses LVM metadata.
-
-Author: Gabriel de Perthuis	      <g2p.code+augeas@gmail.com>
-
-About: License
-  This file is licensed under the LGPL v2+.
-
-About: Configuration files
-  This lens applies to files in /etc/lvm/backup and /etc/lvm/archive.
-
-About: Examples
-  The <Test_LVM> file contains various examples and tests.
-*)
-
-module Guestfs_LVM_conf -	autoload xfm
-
-	(* See lvm2/libdm/libdm-config.c for tokenisation;
-	 * libdm uses a blacklist but I prefer the safer whitelist approach. *)
-	(* View: identifier
-	 * The left hand side of a definition *)
-	let identifier = /[a-zA-Z0-9_-]+/
-
-	(* strings can contain backslash-escaped dquotes, but I don't know
-	 * how to get the message across to augeas *)
-	let str = [label "str". Quote.do_dquote (store
/([^\"]|\\\\.)*/)]
-	let int = [label "int". store Rx.relinteger]
-	(* View: flat_literal
-	 * A literal without structure *)
-	let flat_literal = int|str
-
-	(* allow multiline and mixed int/str, used for raids and stripes *)
-	(* View: list
-	 * A list containing flat literals *)
-	let list = [
-		  label "list" . counter "list"
-		. del /\[[ \t\n]*/ "["
-		.([seq "list". flat_literal . del /,[ \t\n]*/ ", "]*
-				. [seq "list". flat_literal . del /[ \t\n]*/ ""])?
-		. Util.del_str "]"]
-
-	(* View: val
-	 * Any value that appears on the right hand side of an assignment *)
-	let val = flat_literal | list
-
-	(* View: nondef
-	 * A line that doesn't contain a statement *)
-	let nondef -		  Util.empty
-		| Util.comment
-
-	(* Build.block couldn't be reused, because of recursion and
-	 * a different philosophy of whitespace handling. *)
-	(* View: def
-	 * An assignment, or a block containing definitions *)
-	let rec def = [
-		  Util.indent . key identifier . (
-			   del /[ \t]*\{\n/ " {\n"
-			  .[label "dict".(nondef | def)*]
-			  . Util.indent . Util.del_str "}\n"
-			  |Sep.space_equal . val . Util.comment_or_eol)]
-
-	(* View: lns
-	 * The main lens *)
-	let lns = (nondef | def)*
-
-	let filter -		  incl "/etc/lvm/archive/*.vg"
-		. incl "/etc/lvm/backup/*"
-		. Util.stdexcl
-
-	let xfm = transform lns filter
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 0f0d42836..610168ed8 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -255,7 +255,7 @@ extern char *get_blkid_tag (const char *device, const char
*tag);
 extern int lv_canonical (const char *device, char **ret);
 
 /* lvm-filter.c */
-extern void copy_lvm (void);
+extern void clean_lvm_config (void);
 extern void start_lvmetad (void);
 
 /* zero.c */
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 86592e8dc..a209098d4 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -289,7 +289,7 @@ main (int argc, char *argv[])
    * daemon/lvm-filter.c).
    */
   if (!test_mode) {
-    copy_lvm ();
+    clean_lvm_config ();
     start_lvmetad ();
   }
 
diff --git a/daemon/lvm-filter.c b/daemon/lvm-filter.c
index deaecfb67..aff3c68ed 100644
--- a/daemon/lvm-filter.c
+++ b/daemon/lvm-filter.c
@@ -36,67 +36,49 @@
 #include "daemon.h"
 #include "actions.h"
 
-DECLARE_EXTERNAL_COMMANDS ("lvm", "cp", "rm",
"lvmetad")
+DECLARE_EXTERNAL_COMMANDS ("lvm", "rm",
"lvmetad")
 
-/* This runs during daemon start up and creates a complete copy of
- * /etc/lvm so that we can modify it as we desire.  We set
- * LVM_SYSTEM_DIR to point to the copy.  Note that the final directory
- * layout is:
+/* This runs during daemon start up and creates a fresh LVM
+ * configuration which we can modify as we desire.  LVM allows
+ * configuration to be completely empty (meaning "all defaults").
+ *
+ * The final directory layout is:
+ *
  *   /tmp/lvmXXXXXX                 (lvm_system_dir set to this)
  *   /tmp/lvmXXXXXX/lvm             ($LVM_SYSTEM_DIR set to this)
- *   /tmp/lvmXXXXXX/lvm/lvm.conf    (configuration file)
- *   /tmp/lvmXXXXXX/lvm/cache
- *   etc.
+ *   /tmp/lvmXXXXXX/lvm/lvm.conf    (configuration file - initially empty)
  */
 static char lvm_system_dir[] = "/tmp/lvmXXXXXX";
 
 static void rm_lvm_system_dir (void);
+static void debug_lvm_config (void);
 
 void
-copy_lvm (void)
+clean_lvm_config (void)
 {
-  struct stat statbuf;
-  char cmd[64], env[64];
-  int r;
-
-  /* If /etc/lvm directory doesn't exist (or isn't a directory) assume
-   * that this system doesn't support LVM and do nothing.
-   */
-  r = stat ("/etc/lvm", &statbuf);
-  if (r == -1) {
-    perror ("copy_lvm: stat: /etc/lvm");
-    return;
-  }
-  if (! S_ISDIR (statbuf.st_mode)) {
-    fprintf (stderr, "copy_lvm: warning: /etc/lvm is not a
directory\n");
-    return;
-  }
+  char env[64], conf[64];
+  FILE *fp;
 
   if (mkdtemp (lvm_system_dir) == NULL)
     error (EXIT_FAILURE, errno, "mkdtemp: %s", lvm_system_dir);
 
-  /* Copy the entire directory */
-  snprintf (cmd, sizeof cmd, "%s -a /etc/lvm/ %s", "cp",
lvm_system_dir);
-  r = system (cmd);
-  if (r == -1) {
-    perror (cmd);
-    rmdir (lvm_system_dir);
-    exit (EXIT_FAILURE);
-  }
-
-  if (WEXITSTATUS (r) != 0) {
-    fprintf (stderr, "cp command failed with return code %d\n",
-             WEXITSTATUS (r));
-    rmdir (lvm_system_dir);
-    exit (EXIT_FAILURE);
-  }
-
-  /* Set environment variable so we use the copy. */
   snprintf (env, sizeof env, "%s/lvm", lvm_system_dir);
+  mkdir (env, 0755);
+  snprintf (conf, sizeof conf, "%s/lvm/lvm.conf", lvm_system_dir);
+  fp = fopen (conf, "w");
+  if (fp == NULL) {
+    perror ("clean_lvm_config: cannot create empty lvm.conf");
+    exit (EXIT_FAILURE);
+  }
+  fclose (fp);
+
+  /* Set environment variable so we use the clean configuration. */
   setenv ("LVM_SYSTEM_DIR", env, 1);
 
   /* Set a handler to remove the temporary directory at exit. */
   atexit (rm_lvm_system_dir);
+
+  debug_lvm_config ();
 }
 
 /* Try to run lvmetad, without failing if it couldn't. */
@@ -127,98 +109,40 @@ rm_lvm_system_dir (void)
 static int
 set_filter (char *const *filters)
 {
-  CLEANUP_AUG_CLOSE augeas *aug = NULL;
-  int r;
-  int count;
+  const char *filter_types[] = { "filter", "global_filter",
NULL };
+  CLEANUP_FREE char *conf = NULL;
+  FILE *fp;
+  size_t i, j;
 
-  /* Small optimization: do not load the files at init time,
-   * but do that only after having applied the transformation.
-   */
-  const int flags = AUG_NO_ERR_CLOSE | AUG_NO_LOAD;
-  aug = aug_init (lvm_system_dir, "/usr/share/guestfs/", flags);
-  if (!aug) {
-    reply_with_error ("augeas initialization failed");
+  if (asprintf (&conf, "%s/lvm/lvm.conf", lvm_system_dir) == -1)
{
+    reply_with_perror ("asprintf");
     return -1;
   }
-
-  if (aug_error (aug) != AUG_NOERROR) {
-    AUGEAS_ERROR ("aug_init");
-    return -1;
-  }
-
-  r = aug_transform (aug, "guestfs_lvm_conf",
"/lvm/lvm.conf",
-                     0 /* = included */);
-  if (r == -1) {
-    AUGEAS_ERROR ("aug_transform");
-    return -1;
-  }
-
-  if (aug_load (aug) == -1) {
-    AUGEAS_ERROR ("aug_load");
+  fp = fopen (conf, "we");
+  if (fp == NULL) {
+    reply_with_perror ("open: %s", conf);
     return -1;
   }
 
-  /* Remove all the old filters ... */
-  r = aug_rm (aug, "/files/lvm/lvm.conf/devices/dict/filter/list/*");
-  if (r == -1) {
-    AUGEAS_ERROR ("aug_rm/filter");
-    return -1;
-  }
-  r = aug_rm (aug,
"/files/lvm/lvm.conf/devices/dict/global_filter/list/*");
-  if (r == -1) {
-    AUGEAS_ERROR ("aug_rm/global_filter");
-    return -1;
-  }
-
-  /* ... and add the new ones. */
-  for (count = 0; filters[count] != NULL; ++count) {
-    char buf[128];
-
-    snprintf (buf, sizeof buf,
-              "/files/lvm/lvm.conf/devices/dict/filter/list/%d/str",
-              count + 1);
+  fprintf (fp, "devices {\n");
+  for (j = 0; filter_types[j] != NULL; ++j) {
+    fprintf (fp, "    %s = [\n", filter_types[j]);
+    fprintf (fp, "        ");
 
-    if (aug_set (aug, buf, filters[count]) == -1) {
-      AUGEAS_ERROR ("aug_set/filter: %d: %s", count, filters[count]);
-      return -1;
+    for (i = 0; filters[i] != NULL; ++i) {
+      if (i > 0)
+        fprintf (fp, ",\n        ");
+      fprintf (fp, "\"%s\"", filters[i]);
     }
 
-    snprintf (buf, sizeof buf,
-             
"/files/lvm/lvm.conf/devices/dict/global_filter/list/%d/str",
-              count + 1);
-
-    if (aug_set (aug, buf, filters[count]) == -1) {
-      AUGEAS_ERROR ("aug_set/global_filter: %d: %s", count,
filters[count]);
-      return -1;
-    }
+    fprintf (fp, "\n");
+    fprintf (fp, "    ]\n");
   }
+  fprintf (fp, "}\n");
 
-  /* Safety check for the written filter nodes. */
-  r = aug_match (aug,
"/files/lvm/lvm.conf/devices/dict/filter/list/*/str",
-                 NULL);
-  if (r == -1) {
-    AUGEAS_ERROR ("aug_match/filter");
-    return -1;
-  }
-  if (r != count) {
-    reply_with_error ("filters# vs matches mismatch: %d vs %d",
count, r);
-    return -1;
-  }
-  r = aug_match (aug,
"/files/lvm/lvm.conf/devices/dict/global_filter/list/*/str",
-                 NULL);
-  if (r == -1) {
-    AUGEAS_ERROR ("aug_match/global_filter");
-    return -1;
-  }
-  if (r != count) {
-    reply_with_error ("global_filter# vs matches mismatch: %d vs %d",
count, r);
-    return -1;
-  }
+  fclose (fp);
 
-  if (aug_save (aug) == -1) {
-    AUGEAS_ERROR ("aug_save");
-    return -1;
-  }
+  debug_lvm_config ();
 
   return 0;
 }
@@ -269,6 +193,16 @@ rescan (void)
   return 0;
 }
 
+/* Show what lvm thinks is the current config.  Useful for debugging. */
+static void
+debug_lvm_config (void)
+{
+  if (verbose) {
+    fprintf (stderr, "lvm config:\n");
+    ignore_value (system ("lvm config"));
+  }
+}
+
 /* Construct the new, specific filter strings.  We can assume that
  * the 'devices' array does not contain any regexp metachars,
  * because it's already been checked by the stub code.
-- 
2.13.2

No functional change.
---
 tests/lvm/test-lvm-filtering.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/lvm/test-lvm-filtering.sh b/tests/lvm/test-lvm-filtering.sh
index 0c8b8803a..abb88ae6c 100755
--- a/tests/lvm/test-lvm-filtering.sh
+++ b/tests/lvm/test-lvm-filtering.sh
@@ -40,22 +40,22 @@ pvcreate /dev/sdb1
 vgcreate VG1 /dev/sda1
 vgcreate VG2 /dev/sdb1
 
-# Should see VG1 and VG2
+echo Expect VG1, VG2
 vgs
 
-# Should see just VG1
+echo Expect VG1, VG1
 lvm-set-filter /dev/sda
 vgs
 lvm-set-filter /dev/sda1
 vgs
 
-# Should see just VG2
+echo Expect VG2, VG2
 lvm-set-filter /dev/sdb
 vgs
 lvm-set-filter /dev/sdb1
 vgs
 
-# Should see VG1 and VG2
+echo Expect VG1, VG2 x 5
 lvm-set-filter "/dev/sda /dev/sdb"
 vgs
 lvm-set-filter "/dev/sda1 /dev/sdb1"
@@ -69,12 +69,16 @@ vgs
 EOF
 )
 
-expected="VG1
+expected="Expect VG1, VG2
+VG1
 VG2
+Expect VG1, VG1
 VG1
 VG1
+Expect VG2, VG2
 VG2
 VG2
+Expect VG1, VG2 x 5
 VG1
 VG2
 VG1
-- 
2.13.2

Checking out sources from https://github.com/libguestfs/libguestfs ...

/var/tmp/tmp5QHD8R/libguestfs /var/tmp/tmp5QHD8R
Reset branch 'master'
Branch master set up to track remote branch master from origin.
Your branch is up-to-date with 'origin/master'.
Already up-to-date.
/var/tmp/tmp5QHD8R

Applying patches ...

/var/tmp/tmp5QHD8R/libguestfs /var/tmp/tmp5QHD8R
error: patch failed: daemon/daemon.h:255
error: daemon/daemon.h: patch does not apply
error: patch failed: daemon/lvm-filter.c:36
error: daemon/lvm-filter.c: patch does not apply
Applying: daemon: Reimplement handling of lvm.conf and filters.
Patch failed at 0001 daemon: Reimplement handling of lvm.conf and filters.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am
--abort".

Checking out sources from https://github.com/libguestfs/libguestfs ...

/var/tmp/tmpuFgz6c/libguestfs /var/tmp/tmpuFgz6c
Reset branch 'master'
Branch master set up to track remote branch master from origin.
Your branch is up-to-date with 'origin/master'.
Already up-to-date.
/var/tmp/tmpuFgz6c

Applying patches ...

/var/tmp/tmpuFgz6c/libguestfs /var/tmp/tmpuFgz6c
error: patch failed: daemon/daemon.h:255
error: daemon/daemon.h: patch does not apply
error: patch failed: daemon/lvm-filter.c:36
error: daemon/lvm-filter.c: patch does not apply
Applying: daemon: Reimplement handling of lvm.conf and filters.
Patch failed at 0001 daemon: Reimplement handling of lvm.conf and filters.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am
--abort".

On Wed, Jul 26, 2017 at 11:51:05AM -0400, Richard Jones
wrote:
> 
> Checking out sources from https://github.com/libguestfs/libguestfs ...
> 
> /var/tmp/tmp5QHD8R/libguestfs /var/tmp/tmp5QHD8R
> Reset branch 'master'
> Branch master set up to track remote branch master from origin.
> Your branch is up-to-date with 'origin/master'.
> Already up-to-date.
> /var/tmp/tmp5QHD8R
> 
> Applying patches ...
> 
> /var/tmp/tmp5QHD8R/libguestfs /var/tmp/tmp5QHD8R
> error: patch failed: daemon/daemon.h:255
> error: daemon/daemon.h: patch does not apply
> error: patch failed: daemon/lvm-filter.c:36
> error: daemon/lvm-filter.c: patch does not apply
> Applying: daemon: Reimplement handling of lvm.conf and filters.
> Patch failed at 0001 daemon: Reimplement handling of lvm.conf and filters.
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am
--abort".
I should say this depends (not intrinsicly) on the
DECLARE_EXTERNAL_COMMANDS patch set.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v