[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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.

> 
> > +    reply_with_error ("truncated output");
> 
> I'd print out anyway, to ease debugging failures a bit.

Okay.

> 
> > +    return NULL;
> > +  }
> > +
> > +  /* If the path is the btrfs root, `btrfs subvolume show' reports:
> > +   *   <path> is btrfs root
> > +   */
> > +  if (strstr(out, "is btrfs root") != NULL) {
> 
> Take care of adding a space before parenthesis (also in other parts of
> this patch).

Okay.

> 
> > +    reply_with_error ("%s is btrfs root", subvolume);
> > +    return NULL;
> > +  }
> > +
> > +  /* The first line is the path of the subvolume. */
> > +  if (add_string (&ret, strdup("path")) == -1) {
> 
> add_string duplicates the string, so you don't need to do that
> manually.

Okay.

> 
> > +    return NULL;
> > +  }
> > +  if (add_string (&ret, out) == -1) {
> > +    return NULL;
> > +  }
> > +
> > +  /* Read the lines and split into "key: value". */
> > +  while (*p) {
> > +    /* leading spaces and tabs */
> > +    while (*p && c_isspace (*p)) p++;
> 
> Properly indent such lines, so:
>   while (*p && c_isspace (*p))
>     p++;
> 
> (also, a minor optimization is to use ++p instead of p++, as it can
> avoid a temporary value)

Okay.

> 
> > +
> > +    pend = strchrnul (p, '\n');
> > +    if (*pend == '\n') {
> > +      *pend = '\0';
> > +      pend++;
> > +    }
> > +
> > +    if (!*p) continue;
> > +
> > +    colon = strchr (p, ':');
> > +    if (colon) {
> > +      *colon = '\0';
> > +
> > +      /* snapshot is special, see the output above */
> > +      if (strncmp(p, "Snapshot", sizeof("Snapshot") - 1) == 0) {
> 
> Please use the STR* macros we have in src/guestfs-internal-all.h.
> 
> Also, most probably it is better to match "Snapshot(s)", so if in the
> future a new field starting with "Snapshot" is added then it won't be
> mistaken for this multiline one.

Sounds good.

> 
> > +        char *ss = NULL;
> > +
> > +        if (add_string (&ret, p) == -1) {
> > +          return NULL;
> > +        }
> 
> Single statements don't need curly brackets (also in other parts of
> the patch).

I think it is better to document this one, too.

> 
> > +
> > +        /* 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.

> 
> > +          if (ss) {
> > +            ss = realloc(ss, strlen(ss) + strlen(p));
> 
> If realloc fails, the return value is null; this means that the
> following will leak ss in that case.
> 
> Also, sounds like you are missing "," in the length of the string?
> 
> > +            strcat(ss, ",");
> 
> Just save the length of ss before the realloc, and you can simply do
>   ss[old_length_of_ss] = ',';
> 
> > +            strcat(ss, p);
> 
> You can easily use memcpy here:
>   memcpy (ss + old_length_of_ss + 1, p, strlen (p));
> 
> > +          } else {
> > +            ss = strdup(p);
> 
> Missing check for failed strdup.
> 
> > +          }
> > +
> > +          p = pend;
> > +        }
> > +
> > +        if (ss) {
> > +          if (add_string (&ret, ss) == -1) {
> 
> This needs to be add_string_nodup.
> 
> > +            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.

> 
> > +            return NULL;
> > +          }
> > +        }
> > +        else {
> > +          if (add_string (&ret, strdup("")) == -1) {
> > +            return NULL;
> > +          }
> > +        }
> 
> If there are no elements for "Snapshot(s)", then it would be better
> to just not add an empty hash element.
> 
> > +
> > +        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?

> 
> > +    }
> > +    else {
> > +      if (add_string (&ret, p) == -1) {
> > +        return NULL;
> > +      }
> > +      if (add_string (&ret, "") == -1) {
> > +        return NULL;
> > +      }
> 
> As above, I would not add elements with empty values in the hash.
> 
> > +    }
> > +
> > +    p = pend;
> > +  }
> > +
> > +  if (end_stringsbuf (&ret) == -1)
> > +    return NULL;
> > +
> > +  return ret.argv;
> > +
> > +}
> > diff --git a/generator/actions.ml b/generator/actions.ml
> > index cf96039..24d3ecc 100644
> > --- a/generator/actions.ml
> > +++ b/generator/actions.ml
> > @@ -12014,6 +12014,15 @@ This is the internal call which implements C<guestfs_lstatnslist>." };
> >      longdesc = "\
> >  Get the default subvolume or snapshot of a filesystem mounted at C<mountpoint>." };
> >  
> > +  { defaults with
> > +    name = "btrfs_subvolume_show";
> > +    style = RHashtable "btrfssubvolumeinfo", [Pathname "subvolume"], [];
> > +    proc_nr = Some 425;
> > +    optional = Some "btrfs"; camel_name = "BTRFSSubvolumeShow";
> > +    shortdesc = "show detailed information of the subvolume";
> 
> "returns detailed information about the subvolume"
> 
> > +    longdesc = "\
> > +show detailed information of the subvolume." };
> 
> Like above, making sure it is properly capitalized at the beginning.

sure.

Thank you for reviewing!

> 
> -- 
> Pino Toscano


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]