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

On Monday 24 November 2014 16:22:53 Hu Tao wrote: > 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. Well, that's not what would happen: "p" points to the first character of the line after the "Snapshot(s)" one, and strchr() will try to find ':' until the end of the string, not stopping at newlines and such. Let's say the output is something like: 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 Other attribute: - p would be at: snapshots/test1 ^- here, while strchr(p, ':') == NULL) would be Other attribute: - ^- here, so skipping all the multi-line value.

> > > + 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. field pointing at their non-blank start, and value as null.

> > > + 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. I don't understand what you mean here, can you please be more verbose?

> > > + > > > + 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? Do you have an example of them? I haven't used tune2fs-l much myself, but I don't remember having seen empty values in its output.

<fs> mkfs ext3 /dev/sda6 <fs> tune2fs-l /dev/sda6

-- Pino Toscano _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs