Libguestfs - Mar 2015 - Re: [PATCH] btrfs-qgroup-show: add check for "--raw"

btrfs-prog commit:
58a39524619f38d193b8adc3d57888ec07b612aa
change the default output to binary prefix,
and introduced a new option '--raw'
to keep the traditional behaviour.

This patch will add a check function to determine
whether to add '--raw' option to 'btrfs show qgroup'.

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
 daemon/btrfs.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index d4b3207..ddaf15b 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1210,12 +1210,43 @@ do_btrfs_qgroup_destroy (const char *qgroupid, const
char *subvolume)
   return 0;
 }
 
+/* btrfs qgroup show command change default output to
+ * binary prefix since v3.18.2, such as KiB;
+ * also introduced '--raw' to keep traditional behaviour.
+ * We could check wheter 'btrfs qgroup show' support '--raw'
+ * option by checking the output of
+ * 'btrfs qgroup show' support --help' command.
+ */
+static int
+test_btrfs_qgroup_show_raw_opt (void)
+{
+  static int result = -1;
+  if (result > 0)
+    return result;
+
+  CLEANUP_FREE char *err = NULL;
+  CLEANUP_FREE char *out = NULL;
+
+  int r = commandr (&out, &err, str_btrfs, "qgroup",
"show", "--help", NULL);
+
+  if (r == -1) {
+    reply_with_error ("btrfs qgroup show --help: %s", err);
+    return -1;
+  }
+
+  if (!strstr (out, "--raw"))
+      result = 0;
+
+  return result;
+}
+
 guestfs_int_btrfsqgroup_list *
 do_btrfs_qgroup_show (const char *path)
 {
   const size_t MAX_ARGS = 64;
   const char *argv[MAX_ARGS];
   size_t i = 0;
+  int btrfs_qgroup_show_raw_opt = test_btrfs_qgroup_show_raw_opt ();
   CLEANUP_FREE char *path_buf = NULL;
   CLEANUP_FREE char *err = NULL;
   CLEANUP_FREE char *out = NULL;
@@ -1231,6 +1262,8 @@ do_btrfs_qgroup_show (const char *path)
   ADD_ARG (argv, i, str_btrfs);
   ADD_ARG (argv, i, "qgroup");
   ADD_ARG (argv, i, "show");
+  if (btrfs_qgroup_show_raw_opt)
+    ADD_ARG (argv, i, "--raw");
   ADD_ARG (argv, i, path_buf);
   ADD_ARG (argv, i, NULL);
 
-- 
2.1.0

On Sunday 15 March 2015 23:08:07 Chen Hanxiao wrote:
> btrfs-prog commit:
> 58a39524619f38d193b8adc3d57888ec07b612aa
> change the default output to binary prefix,
> and introduced a new option '--raw'
> to keep the traditional behaviour.
> 
> This patch will add a check function to determine
> whether to add '--raw' option to 'btrfs show qgroup'.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> ---
>  daemon/btrfs.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index d4b3207..ddaf15b 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -1210,12 +1210,43 @@ do_btrfs_qgroup_destroy (const char *qgroupid,
const char *subvolume)
>    return 0;
>  }
>  
> +/* btrfs qgroup show command change default output to
> + * binary prefix since v3.18.2, such as KiB;
> + * also introduced '--raw' to keep traditional behaviour.
> + * We could check wheter 'btrfs qgroup show' support
'--raw'
> + * option by checking the output of
> + * 'btrfs qgroup show' support --help' command.
> + */
> +static int
> +test_btrfs_qgroup_show_raw_opt (void)
> +{
> +  static int result = -1;
> +  if (result > 0)
> +    return result;
This should check for result != -1, otherwise if --raw is not supported
this function will be run fully (i.e. executing `btrfs`) every time.
> +  CLEANUP_FREE char *err = NULL;
> +  CLEANUP_FREE char *out = NULL;
> +
> +  int r = commandr (&out, &err, str_btrfs, "qgroup",
"show", "--help", NULL);
> +
> +  if (r == -1) {
> +    reply_with_error ("btrfs qgroup show --help: %s", err);
> +    return -1;
> +  }
> +
> +  if (!strstr (out, "--raw"))
!foo -> foo == NULL (clearer).
>  guestfs_int_btrfsqgroup_list *
>  do_btrfs_qgroup_show (const char *path)
>  {
>    const size_t MAX_ARGS = 64;
>    const char *argv[MAX_ARGS];
>    size_t i = 0;
> +  int btrfs_qgroup_show_raw_opt = test_btrfs_qgroup_show_raw_opt ();
Maybe a shorter name for the local variable will suffice? Something
like has_raw_opt.
>    CLEANUP_FREE char *path_buf = NULL;
>    CLEANUP_FREE char *err = NULL;
>    CLEANUP_FREE char *out = NULL;
> @@ -1231,6 +1262,8 @@ do_btrfs_qgroup_show (const char *path)
>    ADD_ARG (argv, i, str_btrfs);
>    ADD_ARG (argv, i, "qgroup");
>    ADD_ARG (argv, i, "show");
> +  if (btrfs_qgroup_show_raw_opt)
> +    ADD_ARG (argv, i, "--raw");
This should check for > 0, otherwise it will add --raw also when it was
not possible to determine whether --raw is supported (i.e. when the
value is -1).

Thanks,
-- 
Pino Toscano

> -----Original Message-----
> From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com]
On
> Behalf Of Pino Toscano
> Sent: Monday, March 16, 2015 5:24 PM
> To: libguestfs@redhat.com
> Subject: Re: [Libguestfs] [PATCH] btrfs-qgroup-show: add check for
"--raw"
> 
> On Sunday 15 March 2015 23:08:07 Chen Hanxiao wrote:
> > btrfs-prog commit:
> > 58a39524619f38d193b8adc3d57888ec07b612aa
> > change the default output to binary prefix,
> > and introduced a new option '--raw'
> > to keep the traditional behaviour.
> >
> > This patch will add a check function to determine
> > whether to add '--raw' option to 'btrfs show qgroup'.
> >
> > Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> > ---
> >  daemon/btrfs.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> > index d4b3207..ddaf15b 100644
> > --- a/daemon/btrfs.c
> > +++ b/daemon/btrfs.c
> > @@ -1210,12 +1210,43 @@ do_btrfs_qgroup_destroy (const char *qgroupid,
const char
> *subvolume)
> >    return 0;
> >  }
> >
> > +/* btrfs qgroup show command change default output to
> > + * binary prefix since v3.18.2, such as KiB;
> > + * also introduced '--raw' to keep traditional behaviour.
> > + * We could check wheter 'btrfs qgroup show' support
'--raw'
> > + * option by checking the output of
> > + * 'btrfs qgroup show' support --help' command.
> > + */
> > +static int
> > +test_btrfs_qgroup_show_raw_opt (void)
> > +{
> > +  static int result = -1;
> > +  if (result > 0)
> > +    return result;
> 
> This should check for result != -1, otherwise if --raw is not supported
> this function will be run fully (i.e. executing `btrfs`) every time.
Will fix.
> 
> > +  CLEANUP_FREE char *err = NULL;
> > +  CLEANUP_FREE char *out = NULL;
> > +
> > +  int r = commandr (&out, &err, str_btrfs,
"qgroup", "show", "--help", NULL);
> > +
> > +  if (r == -1) {
> > +    reply_with_error ("btrfs qgroup show --help: %s", err);
> > +    return -1;
> > +  }
> > +
> > +  if (!strstr (out, "--raw"))
> 
> !foo -> foo == NULL (clearer).
> 
> >  guestfs_int_btrfsqgroup_list *
> >  do_btrfs_qgroup_show (const char *path)
> >  {
> >    const size_t MAX_ARGS = 64;
> >    const char *argv[MAX_ARGS];
> >    size_t i = 0;
> > +  int btrfs_qgroup_show_raw_opt = test_btrfs_qgroup_show_raw_opt ();
> 
> Maybe a shorter name for the local variable will suffice? Something
> like has_raw_opt.
That's a good name.
> 
> >    CLEANUP_FREE char *path_buf = NULL;
> >    CLEANUP_FREE char *err = NULL;
> >    CLEANUP_FREE char *out = NULL;
> > @@ -1231,6 +1262,8 @@ do_btrfs_qgroup_show (const char *path)
> >    ADD_ARG (argv, i, str_btrfs);
> >    ADD_ARG (argv, i, "qgroup");
> >    ADD_ARG (argv, i, "show");
> > +  if (btrfs_qgroup_show_raw_opt)
> > +    ADD_ARG (argv, i, "--raw");
> 
> This should check for > 0, otherwise it will add --raw also when it was
> not possible to determine whether --raw is supported (i.e. when the
> value is -1).
> 
Sure, v2 will come soon.

Regards,
- Chen