[PATCH 0/3] fix setting lvm filter with newer lvm2
Hi, newer lvm2 releases don't have have uncommented "filter" lines, so the current way to edit lvm.conf doesn't work anymore. Instead, switch to augeas (with a "custom" len) for a cleaner and working way to set the lvm filter. Pino Toscano (3): daemon: add add_sprintf daemon: move AUGEAS_ERROR to the common header daemon: lvm-filter: use augeas for setting the filter appliance/Makefile.am | 6 +- appliance/guestfs_lvm_conf.aug | 74 +++++++++++++++++ daemon/augeas.c | 17 ---- daemon/daemon.h | 19 +++++ daemon/guestfsd.c | 20 +++++ daemon/lvm-filter.c | 183 +++++++++++++++++++++-------------------- 6 files changed, 211 insertions(+), 108 deletions(-) create mode 100644 appliance/guestfs_lvm_conf.aug -- 1.9.3
Bring the add_sprintf function for stringsbuf from the library. --- daemon/daemon.h | 2 ++ daemon/guestfsd.c | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/daemon/daemon.h b/daemon/daemon.h index 0caad45..705aa95 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -89,6 +89,8 @@ struct stringsbuf { */ extern int add_string_nodup (struct stringsbuf *sb, char *str); extern int add_string (struct stringsbuf *sb, const char *str); +extern int add_sprintf (struct stringsbuf *sb, const char *fs, ...) + __attribute__((format (printf,2,3))); extern int end_stringsbuf (struct stringsbuf *sb); extern size_t count_strings (char *const *argv); diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 0a59c84..321544f 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -560,6 +560,26 @@ add_string (struct stringsbuf *sb, const char *str) } int +add_sprintf (struct stringsbuf *sb, const char *fs, ...) +{ + va_list args; + char *str; + int r; + + va_start (args, fs); + r = vasprintf (&str, fs, args); + va_end (args); + if (r == -1) { + reply_with_perror ("vasprintf"); + free_stringslen (sb->argv, sb->size); + sb->argv = NULL; + return -1; + } + + return add_string_nodup (sb, str); +} + +int end_stringsbuf (struct stringsbuf *sb) { return add_string_nodup (sb, NULL); -- 1.9.3
Other than for current aug_* API, it will be useful for further code using augeas directly. --- daemon/augeas.c | 17 ----------------- daemon/daemon.h | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/daemon/augeas.c b/daemon/augeas.c index e31cab4..74f3ba7 100644 --- a/daemon/augeas.c +++ b/daemon/augeas.c @@ -55,23 +55,6 @@ aug_finalize (void) } \ while (0) -/* Calls reply_with_error, but includes the Augeas error details. */ -#define AUGEAS_ERROR(fs,...) \ - do { \ - int code = aug_error (aug); \ - if (code == AUG_ENOMEM) \ - reply_with_error (fs ": augeas out of memory", ##__VA_ARGS__); \ - else { \ - const char *message = aug_error_message (aug); \ - const char *minor = aug_error_minor_message (aug); \ - const char *details = aug_error_details (aug); \ - reply_with_error (fs ": %s%s%s%s%s", ##__VA_ARGS__, \ - message, \ - minor ? ": " : "", minor ? minor : "", \ - details ? ": " : "", details ? details : ""); \ - } \ - } while (0) - /* We need to rewrite the root path so it is based at /sysroot. */ int do_aug_init (const char *root, int flags) diff --git a/daemon/daemon.h b/daemon/daemon.h index 705aa95..d90b3e7 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -403,6 +403,23 @@ is_zero (const char *buffer, size_t size) } \ while (0) +/* Calls reply_with_error, but includes the Augeas error details. */ +#define AUGEAS_ERROR(fs,...) \ + do { \ + int code = aug_error (aug); \ + if (code == AUG_ENOMEM) \ + reply_with_error (fs ": augeas out of memory", ##__VA_ARGS__); \ + else { \ + const char *message = aug_error_message (aug); \ + const char *minor = aug_error_minor_message (aug); \ + const char *details = aug_error_details (aug); \ + reply_with_error (fs ": %s%s%s%s%s", ##__VA_ARGS__, \ + message, \ + minor ? ": " : "", minor ? minor : "", \ + details ? ": " : "", details ? details : ""); \ + } \ + } while (0) + #ifdef HAVE_ATTRIBUTE_CLEANUP #define CLEANUP_FREE __attribute__((cleanup(cleanup_free))) #define CLEANUP_FREE_STRING_LIST \ -- 1.9.3
The way to set the filter for lvm devices was to open lvm.conf, look for uncommented "filter =" lines and replace the configuration there. This had the issue that if there is no uncommented filter line, then the filter cannot be changed at all; considering newer lvm2 releases ship a sample configuration with no uncommented filter lines, then the old way became wrong and not sufficient. Instead, take a copy of the upstream lvm.aug lens, with a simple change to allow parsing also negative values in configuration, and install it in the daemon. When asking to change the lvm filter, use augeas making sure to use this custom lens for the lvm.conf copy used within the appliance. --- appliance/Makefile.am | 6 +- appliance/guestfs_lvm_conf.aug | 74 +++++++++++++++++ daemon/lvm-filter.c | 183 +++++++++++++++++++++-------------------- 3 files changed, 172 insertions(+), 91 deletions(-) create mode 100644 appliance/guestfs_lvm_conf.aug diff --git a/appliance/Makefile.am b/appliance/Makefile.am index 418a6f6..bb0c0e8 100644 --- a/appliance/Makefile.am +++ b/appliance/Makefile.am @@ -21,6 +21,7 @@ EXTRA_DIST = \ 99-guestfs-serial.rules \ excludefiles.in \ guestfsd.suppressions \ + guestfs_lvm_conf.aug \ hostfiles.in \ init \ libguestfs-make-fixed-appliance.in \ @@ -74,12 +75,13 @@ packagelist: packagelist.in Makefile cmp -s $@ $@-t || mv $@-t $@ rm -f $@-t -supermin.d/daemon.tar.gz: ../daemon/guestfsd guestfsd.suppressions +supermin.d/daemon.tar.gz: ../daemon/guestfsd guestfsd.suppressions guestfs_lvm_conf.aug rm -f $@ $@-t rm -rf tmp-d - mkdir -p tmp-d$(DAEMON_SUPERMIN_DIR) tmp-d/etc + 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)/guestfsd.suppressions tmp-d/etc/guestfsd.suppressions + ln $(srcdir)/guestfs_lvm_conf.aug tmp-d/usr/share/guestfs/guestfs_lvm_conf.aug ( cd tmp-d && tar zcf - * ) > $@-t rm -r tmp-d mv $@-t $@ diff --git a/appliance/guestfs_lvm_conf.aug b/appliance/guestfs_lvm_conf.aug new file mode 100644 index 0000000..ffa5b01 --- /dev/null +++ b/appliance/guestfs_lvm_conf.aug @@ -0,0 +1,74 @@ +(* +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/lvm-filter.c b/daemon/lvm-filter.c index 6976bab..3bab9bf 100644 --- a/daemon/lvm-filter.c +++ b/daemon/lvm-filter.c @@ -25,12 +25,30 @@ #include <unistd.h> #include <sys/stat.h> +#include <augeas.h> + #include "c-ctype.h" #include "ignore-value.h" #include "daemon.h" #include "actions.h" +#ifdef HAVE_ATTRIBUTE_CLEANUP +#define CLEANUP_AUG_CLOSE __attribute__((cleanup(cleanup_aug_close))) + +static void +cleanup_aug_close (void *ptr) +{ + augeas *aug = * (augeas **) ptr; + + if (aug != NULL) + aug_close (aug); +} + +#else +#define CLEANUP_AUG_CLOSE +#endif + GUESTFSD_EXT_CMD(str_lvm, lvm); GUESTFSD_EXT_CMD(str_cp, cp); GUESTFSD_EXT_CMD(str_rm, rm); @@ -107,87 +125,76 @@ rm_lvm_system_dir (void) ignore_value (system (cmd)); } -/* Does the current line match the regexp /^\s*filter\s*=/ */ +/* Rewrite the 'filter = [ ... ]' line in lvm.conf. */ static int -is_filter_line (const char *line) +set_filter (char *const *filters) { - while (*line && c_isspace (*line)) - line++; - if (!*line) - return 0; - - if (! STRPREFIX (line, "filter")) - return 0; - line += 6; - - while (*line && c_isspace (*line)) - line++; - if (!*line) - return 0; - - if (*line != '=') - return 0; + CLEANUP_AUG_CLOSE augeas *aug = NULL; + int r; + int count; - return 1; -} + /* 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"); + return -1; + } -/* Rewrite the 'filter = [ ... ]' line in lvm.conf. */ -static int -set_filter (const char *filter) -{ - char lvm_conf[64]; - snprintf (lvm_conf, sizeof lvm_conf, "%s/lvm/lvm.conf", lvm_system_dir); + if (aug_error (aug) != AUG_NOERROR) { + AUGEAS_ERROR ("aug_init"); + return -1; + } - char lvm_conf_new[64]; - snprintf (lvm_conf_new, sizeof lvm_conf, "%s/lvm/lvm.conf.new", - lvm_system_dir); + r = aug_transform (aug, "guestfs_lvm_conf", "/lvm/lvm.conf", + 0 /* = included */); + if (r == -1) { + AUGEAS_ERROR ("aug_transform"); + return -1; + } - FILE *ifp = fopen (lvm_conf, "r"); - if (ifp == NULL) { - reply_with_perror ("open: %s", lvm_conf); + if (aug_load (aug) == -1) { + AUGEAS_ERROR ("aug_load"); return -1; } - FILE *ofp = fopen (lvm_conf_new, "w"); - if (ofp == NULL) { - reply_with_perror ("open: %s", lvm_conf_new); - fclose (ifp); + + /* Remove all the old filters ... */ + r = aug_rm (aug, "/files/lvm/lvm.conf/devices/dict/filter/list/*"); + if (r == -1) { + AUGEAS_ERROR ("aug_rm"); return -1; } - CLEANUP_FREE char *line = NULL; - size_t allocsize = 0; - while (getline (&line, &allocsize, ifp) != -1) { - int r; - if (is_filter_line (line)) { - r = fprintf (ofp, " filter = [ %s ]\n", filter); - } else { - r = fprintf (ofp, "%s", line); - } - if (r < 0) { - /* NB. fprintf doesn't set errno on error. */ - reply_with_error ("%s: write failed", lvm_conf_new); - fclose (ifp); - fclose (ofp); - unlink (lvm_conf_new); + /* ... 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); + + if (aug_set (aug, buf, filters[count]) == -1) { + AUGEAS_ERROR ("aug_set: %d: %s", count, filters[count]); return -1; } } - if (fclose (ifp) == EOF) { - reply_with_perror ("close: %s", lvm_conf); - unlink (lvm_conf_new); - fclose (ofp); + /* 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"); return -1; } - if (fclose (ofp) == EOF) { - reply_with_perror ("close: %s", lvm_conf_new); - unlink (lvm_conf_new); + if (r != count) { + reply_with_error ("filters# vs matches mismatch: %d vs %d", count, r); return -1; } - if (rename (lvm_conf_new, lvm_conf) == -1) { - reply_with_perror ("rename: %s", lvm_conf); - unlink (lvm_conf_new); + if (aug_save (aug) == -1) { + AUGEAS_ERROR ("aug_save"); return -1; } @@ -240,25 +247,16 @@ rescan (void) return 0; } -/* Construct the new, specific filter string. We can assume that +/* 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. */ -static char * -make_filter_string (char *const *devices) +static char ** +make_filter_strings (char *const *devices) { size_t i; - size_t len = 64; - for (i = 0; devices[i] != NULL; ++i) - len += 2 * strlen (devices[i]) + 64; - - char *filter = malloc (len); - if (filter == NULL) { - reply_with_perror ("malloc"); - return NULL; - } + DECLARE_STRINGSBUF (ret); - char *p = filter; for (i = 0; devices[i] != NULL; ++i) { /* Because of the way matching works in LVM (yes, they wrote their * own regular expression engine!), each match clause should be either: @@ -269,33 +267,38 @@ make_filter_string (char *const *devices) * "a|^/dev/sda$|", "a|^/dev/sda[0-9]|", */ size_t slen = strlen (devices[i]); - char str[2*slen+64]; - if (c_isdigit (devices[i][slen-1])) /* single partition */ - snprintf (str, 2*slen+64, "\"a|^%s$|\", ", devices[i]); - else /* whole block device */ - snprintf (str, 2*slen+64, "\"a|^%s$|\", \"a|^%s[0-9]|\", ", - devices[i], devices[i]); + if (add_sprintf (&ret, "a|^%s$|", devices[i]) == -1) + goto error; - strcpy (p, str); - p += strlen (str); + if (!c_isdigit (devices[i][slen-1])) { + /* whole block device */ + if (add_sprintf (&ret, "a|^%s[0-9]|", devices[i]) == -1) + goto error; + } } - strcpy (p, "\"r|.*|\""); - - return filter; /* Caller must free. */ + if (add_string (&ret, "r|.*|") == -1) + goto error; + + end_stringsbuf (&ret); + return ret.argv; +error: + if (ret.argv) + free_stringslen (ret.argv, ret.size); + return NULL; } int do_lvm_set_filter (char *const *devices) { - CLEANUP_FREE char *filter = make_filter_string (devices); - if (filter == NULL) + CLEANUP_FREE_STRING_LIST char **filters = make_filter_strings (devices); + if (filters == NULL) return -1; if (deactivate () == -1) return -1; - int r = set_filter (filter); + int r = set_filter (filters); if (r == -1) return -1; @@ -308,10 +311,12 @@ do_lvm_set_filter (char *const *devices) int do_lvm_clear_filter (void) { + const char *const filters[2] = { "a/.*/", NULL }; + if (deactivate () == -1) return -1; - if (set_filter ("\"a/.*/\"") == -1) + if (set_filter ((char *const *) filters) == -1) return -1; if (rescan () == -1) -- 1.9.3
Hi, On Tuesday 26 August 2014 16:29:11 Pino Toscano wrote:
Instead, take a copy of the upstream lvm.aug lens, with a simple change to allow parsing also negative values in configuration, and install it in the daemon.
The improvements to the upstream lvm.aug lens were proposed upstream as https://github.com/hercules-team/augeas/pull/155 -- Pino Toscano
On Tue, Aug 26, 2014 at 04:29:08PM +0200, Pino Toscano wrote:
Hi,
newer lvm2 releases don't have have uncommented "filter" lines, so the current way to edit lvm.conf doesn't work anymore. Instead, switch to augeas (with a "custom" len) for a cleaner and working way to set the lvm filter.
Pino Toscano (3): daemon: add add_sprintf daemon: move AUGEAS_ERROR to the common header daemon: lvm-filter: use augeas for setting the filter
appliance/Makefile.am | 6 +- appliance/guestfs_lvm_conf.aug | 74 +++++++++++++++++ daemon/augeas.c | 17 ---- daemon/daemon.h | 19 +++++ daemon/guestfsd.c | 20 +++++ daemon/lvm-filter.c | 183 +++++++++++++++++++++--------------------
ACK series. Let's remove the custom augeas lens as soon as we can though ... Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Hi, On Tuesday 26 August 2014 16:29:08 Pino Toscano wrote:
newer lvm2 releases don't have have uncommented "filter" lines,
Just for reference: the upstream commit is f76879b [1], which is in lvm2 >= 2.02.108. [1] https://git.fedorahosted.org/cgit/lvm2.git/commit/?id=f76879ba440aa93f2e2373... -- Pino Toscano
participants (2)
-
Pino Toscano -
Richard W.M. Jones