[Libguestfs] [PATCH] btrfs: Fix btrfs_subvolume_list on F18
Richard W.M. Jones
rjones at redhat.com
Thu Jan 24 12:26:45 UTC 2013
On Thu, Jan 24, 2013 at 12:16:39PM +0000, Matthew Booth wrote:
> The output of btrfs subvolume list has changed in F18 to include generation,
> which breaks the parsing in btrfs_subvolume_list. This change replaces sscanf
> with a more robust regular expression. The new regular expression should also
> handle the addition of future unexpected columns.
>
> Fixes RHBZ#903620
> ---
> daemon/Makefile.am | 6 +++--
> daemon/btrfs.c | 67 ++++++++++++++++++++++++++++++++++++++++++------------
> 2 files changed, 56 insertions(+), 17 deletions(-)
>
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index a05771e..1fe8e12 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -197,14 +197,16 @@ guestfsd_LDADD = \
> $(LIBSOCKET) \
> $(LIB_CLOCK_GETTIME) \
> $(LIBINTL) \
> - $(SERVENT_LIB)
> + $(SERVENT_LIB) \
> + $(PCRE_LIBS)
>
> guestfsd_CPPFLAGS = -I$(top_srcdir)/gnulib/lib -I$(top_builddir)/gnulib/lib
> guestfsd_CFLAGS = \
> $(WARN_CFLAGS) $(WERROR_CFLAGS) \
> $(AUGEAS_CFLAGS) \
> $(HIVEX_CFLAGS) \
> - $(YAJL_CFLAGS)
> + $(YAJL_CFLAGS) \
> + $(PCRE_CFLAGS)
Indentation seems to be broken in these two hunks.
> # Manual pages and HTML files for the website.
> man_MANS = guestfsd.8
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 8ecde01..a940f0c 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -21,6 +21,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <inttypes.h>
> +#include <pcre.h>
> #include <string.h>
> #include <unistd.h>
>
> @@ -326,7 +327,7 @@ do_btrfs_subvolume_list (const char *fs)
> char *fs_buf;
> const char *argv[MAX_ARGS];
> size_t i = 0;
> - char *out, *err, **lines, *pos;
> + char *out, *err, **lines;
> size_t nr_subvolumes;
> int r;
>
> @@ -358,13 +359,18 @@ do_btrfs_subvolume_list (const char *fs)
>
> /* Output is:
> *
> - * ID 256 top level 5 path test1
> - * ID 257 top level 5 path dir/test2
> - * ID 258 top level 5 path test3
> + * ID 256 gen 30 top level 5 path test1
> + * ID 257 gen 30 top level 5 path dir/test2
> + * ID 258 gen 30 top level 5 path test3
> *
> - * "ID <n>" is the subvolume ID. "top level <n>" is the top level
> - * subvolume ID. "path <str>" is the subvolume path, relative to
> - * the top of the filesystem.
> + * "ID <n>" is the subvolume ID.
> + * "gen <n>" is the generation when the root was created or last updated.
> + * "top level <n>" is the top level subvolume ID.
> + * "path <str>" is the subvolume path, relative to the top of the filesystem.
> + *
> + * Note that the order that each of the above is fixed, but different versions
> + * of btrfs may display different sets of data. Specifically, older versions
> + * of btrfs do not display gen.
> */
Can you make these lines a little shorter. On emacs, M-q will format
the paragraph correctly.
> nr_subvolumes = count_strings (lines);
>
> @@ -384,33 +390,64 @@ do_btrfs_subvolume_list (const char *fs)
> return NULL;
> }
>
> + const char *errptr;
> + int erroffset;
> + pcre *re = pcre_compile ("ID\\s+(\\d+).*\\s"
> + "top level\\s+(\\d+).*\\s"
> + "path\\s(.*)",
> + 0, &errptr, &erroffset, NULL);
> + if (re == NULL) {
> + reply_with_error ("pcre_compile (%i): %s", erroffset, errptr);
> + free (ret->guestfs_int_btrfssubvolume_list_val);
> + free (ret);
> + free_stringslen (lines, nr_subvolumes);
> + return NULL;
> + }
This whole function is crying out for a common return path. The
current code 3 separate places where stuff is freed on error, and this
adds another one. Not that this is your fault or anything, but it
would surely be an improvement in the code if you could make this
change.
> for (i = 0; i < nr_subvolumes; ++i) {
> /* To avoid allocations, reuse the 'line' buffer to store the
> * path. Thus we don't need to free 'line', since it will be
> * freed by the calling (XDR) code.
> */
> char *line = lines[i];
> + #define N_MATCHES 3
> + int ovector[N_MATCHES * 3];
>
> - if (sscanf (line, "ID %" SCNu64 " top level %" SCNu64 " path ",
> - &ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_id,
> - &ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_top_level_id) != 2) {
> + if (pcre_exec (re, NULL, line, strlen(line), 0, 0,
> + ovector, N_MATCHES * 3) < 0)
> + #undef N_MATCHES
> + {
> unexpected_output:
> reply_with_error ("unexpected output from 'btrfs subvolume list' command: %s", line);
> free_stringslen (lines, nr_subvolumes);
> free (ret->guestfs_int_btrfssubvolume_list_val);
> free (ret);
> + pcre_free(re);
> return NULL;
> }
>
> - pos = strstr (line, " path ");
> - if (pos == NULL)
> - goto unexpected_output;
> - pos += 6;
> + #if __WORDSIZE == 64
> + #define STRTOU64 strtoul
> + #else
> + #define STRTOU64 strtoull
> + #endif
> +
> + errno = 0;
> + ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_id =
> + STRTOU64(line + ovector[0], NULL, 10);
> + if (errno == ERANGE) goto unexpected_output;
> + ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_top_level_id =
> + STRTOU64(line + ovector[2], NULL, 10);
> + if (errno == ERANGE) goto unexpected_output;
I have a feeling if Meyering was around he'd say that you should be
using 'xstrtol' from gnulib. Compare the code in 'src/inspect-fs.c'
for example. You could also use sscanf here ...
> - memmove (line, pos, strlen (pos) + 1);
> + #undef STRTOU64
> +
> + memmove (line, line + ovector[4], ovector[5] + 1);
> ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path = line;
> }
>
> + pcre_free(re);
> +
> return ret;
> }
>
> --
> 1.8.1
You also need to add pcre (or whatever the library package is called
on Fedora / Debian / Arch) to the package list.
Rich.
--
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#)
More information about the Libguestfs
mailing list