[Libguestfs] [PATCH 6/6] New API: btrfs_subvolume_show
Pino Toscano
ptoscano at redhat.com
Mon Nov 24 17:17:22 UTC 2014
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 at cn.fujitsu.com>
> > > ---
> > >
> > > 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.
--
Pino Toscano
More information about the Libguestfs
mailing list