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


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