Re: [Libguestfs] [PATCH 6/6] New API: btrfs_subvolume_show

On Fri, Nov 21, 2014 at 05:17:13PM +0100, Pino Toscano wrote: > On Friday 21 November 2014 13:18:00 Hu Tao wrote: > > btrfs_subvolume_show shows the detailed information of a subvolume > > or > > snapshot. > > > > Signed-off-by: Hu Tao <hutao(a)cn.fujitsu.com&gt; > > --- > > > > daemon/btrfs.c | 167 > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > generator/actions.ml | 9 +++ > > src/MAX_PROC_NR | 2 +- > > 3 files changed, 177 insertions(+), 1 deletion(-) > > > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > > index e179b57..36ba588 100644 > > --- a/daemon/btrfs.c > > +++ b/daemon/btrfs.c > > @@ -29,6 +29,7 @@ > > > > #include "actions.h" > > #include "optgroups.h" > > #include "xstrtol.h" > > > > +#include "c-ctype.h" > > > > GUESTFSD_EXT_CMD(str_btrfs, btrfs); > > GUESTFSD_EXT_CMD(str_btrfstune, btrfstune); > > > > @@ -813,3 +814,169 @@ do_btrfs_fsck (const char *device, int64_t > > superblock, int repair)> > > > return 0; > > > > } > > > > + > > +char **do_btrfs_subvolume_show (const char *subvolume) > > +{ > > + const size_t MAX_ARGS = 64; > > + const char *argv[MAX_ARGS]; > > + size_t i = 0; > > + CLEANUP_FREE char *subvolume_buf = NULL; > > + CLEANUP_FREE char *err = NULL; > > + CLEANUP_FREE char *out = NULL; > > + char *p, *pend, *colon; > > + DECLARE_STRINGSBUF (ret); > > + int r; > > + > > + subvolume_buf = sysroot_path (subvolume); > > + if (subvolume_buf == NULL) { > > + reply_with_perror ("malloc"); > > + return NULL; > > + } > > + > > + ADD_ARG (argv, i, str_btrfs); > > + ADD_ARG (argv, i, "subvolume"); > > + ADD_ARG (argv, i, "show"); > > + ADD_ARG (argv, i, subvolume_buf); > > + ADD_ARG (argv, i, NULL); > > + > > + r = commandv (&out, &err, argv); > > + if (r == -1) { > > + reply_with_error ("%s: %s", subvolume, err); > > + return NULL; > > + } > > + > > + /* Output is: > > + * > > + * / > > + * Name: root > > + * uuid: > > c875169e-cf4e-a04d-9959-b667dec36234 + * Parent uuid: > > - > > + * Creation time: 2014-11-13 10:13:08 > > + * Object ID: 256 > > + * Generation (Gen): 6579 > > + * Gen at creation: 5 > > + * Parent: 5 > > + * Top Level: 5 > > + * Flags: - > > + * Snapshot(s): > > + * snapshots/test1 > > + * snapshots/test2 > > + * snapshots/test3 > > + * > > + */ > > + > > + p = out; > > + > > + p = strchr (p, '\n'); > > + if (p) { > > + *p = '\0'; > > + p++; > > + } > > + else { > > Minor style note: > } else { > > (also in other parts of this patch) Though I'm fine with it, but do we have coding style rules? I see both styles in existing codes.

From what I see, «} else {» is more common.

> > + > > + /* each line is the path of a snapshot. */ > > + p = pend; > > + while (*p && strchr(p, ':') == NULL) { > > This will break once the output will have fields after Snapshot, as > strchr(p, ':') will return a non-null value in one of the lines > following the first line after the Snapshot one. It is intended, so that we can parse properly possibly key:value pairs following "Snapshot(s)". Although a ':' in a snapshot path will hit it too, but people don't likely put a ':' in path.

> > + while (*p && c_isspace (*p)) p++; > > + pend = strchrnul (p, '\n'); > > + if (*pend == '\n') { > > + *pend = '\0'; > > + pend++; > > + } > > I see this code repeated already; I think it would be better to > create> > a small helper function like: > char *analyze_line (char *p, char **field, char **value) > > which would > - destructively update p (setting null bytes, just like it is done now) > - return in field the start of the field text > - return in value, if present, the start of the value (after the colon) > - return the pointer to the first character of the new line (or null, > if none following) > > this way it is IMHO easier to iterate over the lines, and also to > detect whether a line contains a field+value or just text. How to treat lines don't containing a colon? treat them as invalid line? or as value of key in previous line? Note this should be a general process.

> > + free(ss); > > No need to free ss here; theoretically add_string will take care of > it, be it added to the stringbuf or freed on error. This is fail case.

> > + > > + continue; > > + } > > + > > + do { colon++; } while (*colon && c_isspace (*colon)); > > + if (add_string (&ret, p) == -1) { > > + return NULL; > > + } > > + if (add_string (&ret, colon) == -1) { > > + return NULL; > > + } > > At least from the example output added as comment, it seems that > "not available" values might be "-" -- it'd just not add those, > since > as users of this API would still need to check for the existency of > a certain key in the returned hash. tune2fs-l lists keys with empty values, shouldn't we follow it?