Libguestfs - Nov 2014 - Re: [PATCH 5/6] New API: btrfs_subvolume_get_default

On Friday 21 November 2014 13:17:59 Hu Tao wrote:
> btrfs_subvolume_get_default is for getting the default subvolume of
> a btrfs filesystem.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  daemon/btrfs.c       | 38 ++++++++++++++++++++++++++++++++++++++
>  generator/actions.ml |  9 +++++++++
>  src/MAX_PROC_NR      |  2 +-
>  3 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 0f525fa..e179b57 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -549,6 +549,44 @@ do_btrfs_subvolume_set_default (int64_t id, const 
char *fs)
>  }
>  
>  int
> +do_btrfs_subvolume_get_default (const char *fs)
> +{
> +  const size_t MAX_ARGS = 64;
> +  const char *argv[MAX_ARGS];
> +  size_t i = 0;
> +  CLEANUP_FREE char *fs_buf = NULL;
> +  CLEANUP_FREE char *err = NULL;
> +  CLEANUP_FREE char *out = NULL;
> +  int r;
> +
> +  fs_buf = sysroot_path (fs);
> +  if (fs_buf == NULL) {
> +    reply_with_perror ("malloc");
> +    return -1;
> +  }
> +
> +  ADD_ARG (argv, i, str_btrfs);
> +  ADD_ARG (argv, i, "subvolume");
> +  ADD_ARG (argv, i, "get-default");
> +  ADD_ARG (argv, i, fs_buf);
> +  ADD_ARG (argv, i, NULL);
> +
> +  r = commandv (&out, &err, argv);
> +  fprintf(stderr, "%s\n", out);
This looks like a leftover? Otherwise, print it only when "debug" is
enabled (if (debug) ...).
> +  if (r == -1) {
> +    reply_with_error ("%s: %s", fs, err);
> +    return -1;
> +  }
> +  r = -1;
Most probably it is not needed to manually reset r.
> +  if (sscanf (out, "ID %" SCNi32, &r) != 1) {
It would be simplier to just use xstrtol to parse the output.
> +    reply_with_error ("%s: expected output, but got nothing", 
argv[0]);

More than "nothing", "not a number". I'd also append the
whole output
here.
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 1c1fcff..cf96039 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -12005,6 +12005,15 @@ This is the same as the C<lstat(2)> system 
call." };
>      longdesc = "\
>  This is the internal call which implements
C<guestfs_lstatnslist>."
};
>  
> +  { defaults with
> +    name = "btrfs_subvolume_get_default";
> +    style = RInt "id", [Pathname "mountpoint"], [];
> +    proc_nr = Some 424;
> +    optional = Some "btrfs"; camel_name =
"BTRFSSubvolumeGetDefault";
> +    shortdesc = "get the default subvolume or snapshot of a 
filesystem";
> +    longdesc = "\
> +Get the default subvolume or snapshot of a filesystem mounted at 
C<mountpoint>." };
> +
>  ]
>  
>  (* Non-API meta-commands available only in guestfish.
> diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
> index 5721413..9524ef4 100644
> --- a/src/MAX_PROC_NR
> +++ b/src/MAX_PROC_NR
> @@ -1 +1 @@
> -423
> +424
> -- 
> 1.9.3
> 
-- 
Pino Toscano

On Fri, Nov 21, 2014 at 12:11:36PM +0100, Pino Toscano
wrote:
> On Friday 21 November 2014 13:17:59 Hu Tao wrote:
> > btrfs_subvolume_get_default is for getting the default subvolume of
> > a btrfs filesystem.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  daemon/btrfs.c       | 38 ++++++++++++++++++++++++++++++++++++++
> >  generator/actions.ml |  9 +++++++++
> >  src/MAX_PROC_NR      |  2 +-
> >  3 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> > index 0f525fa..e179b57 100644
> > --- a/daemon/btrfs.c
> > +++ b/daemon/btrfs.c
> > @@ -549,6 +549,44 @@ do_btrfs_subvolume_set_default (int64_t id, const
> char *fs)
> >  }
> >  
> >  int
> > +do_btrfs_subvolume_get_default (const char *fs)
> > +{
> > +  const size_t MAX_ARGS = 64;
> > +  const char *argv[MAX_ARGS];
> > +  size_t i = 0;
> > +  CLEANUP_FREE char *fs_buf = NULL;
> > +  CLEANUP_FREE char *err = NULL;
> > +  CLEANUP_FREE char *out = NULL;
> > +  int r;
> > +
> > +  fs_buf = sysroot_path (fs);
> > +  if (fs_buf == NULL) {
> > +    reply_with_perror ("malloc");
> > +    return -1;
> > +  }
> > +
> > +  ADD_ARG (argv, i, str_btrfs);
> > +  ADD_ARG (argv, i, "subvolume");
> > +  ADD_ARG (argv, i, "get-default");
> > +  ADD_ARG (argv, i, fs_buf);
> > +  ADD_ARG (argv, i, NULL);
> > +
> > +  r = commandv (&out, &err, argv);
> > +  fprintf(stderr, "%s\n", out);
> 
> This looks like a leftover? Otherwise, print it only when "debug"
is
> enabled (if (debug) ...).
Oops, I will just remove the line.
> 
> > +  if (r == -1) {
> > +    reply_with_error ("%s: %s", fs, err);
> > +    return -1;
> > +  }
> > +  r = -1;
> 
> Most probably it is not needed to manually reset r.
> 
> > +  if (sscanf (out, "ID %" SCNi32, &r) != 1) {
> 
> It would be simplier to just use xstrtol to parse the output.
Then we should skip "ID " firstly.
> 
> > +    reply_with_error ("%s: expected output, but got
nothing",
> argv[0]);
> 
> More than "nothing", "not a number". I'd also
append the whole output
> here.
Sounds good.
> 
> > diff --git a/generator/actions.ml b/generator/actions.ml
> > index 1c1fcff..cf96039 100644
> > --- a/generator/actions.ml
> > +++ b/generator/actions.ml
> > @@ -12005,6 +12005,15 @@ This is the same as the C<lstat(2)>
system
> call." };
> >      longdesc = "\
> >  This is the internal call which implements
C<guestfs_lstatnslist>."
> };
> >  
> > +  { defaults with
> > +    name = "btrfs_subvolume_get_default";
> > +    style = RInt "id", [Pathname "mountpoint"],
[];
> > +    proc_nr = Some 424;
> > +    optional = Some "btrfs"; camel_name =
"BTRFSSubvolumeGetDefault";
> > +    shortdesc = "get the default subvolume or snapshot of a 
> filesystem";
> > +    longdesc = "\
> > +Get the default subvolume or snapshot of a filesystem mounted at 
> C<mountpoint>." };
> > +
> >  ]
> >  
> >  (* Non-API meta-commands available only in guestfish.
> > diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
> > index 5721413..9524ef4 100644
> > --- a/src/MAX_PROC_NR
> > +++ b/src/MAX_PROC_NR
> > @@ -1 +1 @@
> > -423
> > +424
> > -- 
> > 1.9.3
> > 
> -- 
> Pino Toscano

On Monday 24 November 2014 13:44:11 Hu Tao wrote:
> > > +  if (sscanf (out, "ID %" SCNi32, &r) != 1) {
> > 
> > It would be simplier to just use xstrtol to parse the output.
> 
> Then we should skip "ID " firstly.
Right (got focused on the integer parsing, and lost track of the "ID "
prefix).

Then I guess sscanf could stay as you wrote it.

-- 
Pino Toscano