thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH] New API: btrfs_filesystem_show

If this information is useful, please help other people find it:
Share via:

Chen Hanxiao

2015-Jun-11 04:24 UTC

[Libguestfs] [PATCH] New API: btrfs_filesystem_show_all

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
 daemon/btrfs.c       | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++
 generator/actions.ml |  19 ++++++
 generator/structs.ml |  13 ++++
 src/MAX_PROC_NR      |   2 +-
 4 files changed, 209 insertions(+), 1 deletion(-)

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 39392f7..09f7615 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -2083,3 +2083,179 @@ do_btrfs_image (char *const *sources, const char *image,
 
   return 0;
 }
+
+guestfs_int_btrfsfsshow_list *
+do_btrfs_filesystem_show_all (void)
+{
+  const size_t MAX_ARGS = 64;
+  const char *argv[MAX_ARGS];
+  size_t i = 0;
+  size_t nr_filesystem_show = 0;
+  CLEANUP_FREE char *err = NULL;
+  char *out = NULL;
+  char **lines;
+  int r;
+  /* Info from Label line will be reused for the following devid line
+   * make two temp space to hold them */
+  char *label_tmp = NULL, *uuid_tmp = NULL;
+
+  ADD_ARG (argv, i, str_btrfs);
+  ADD_ARG (argv, i, "filesystem");
+  ADD_ARG (argv, i, "show");
+  ADD_ARG (argv, i, NULL);
+
+  r = commandv (&out, &err, argv);
+  if (r == -1) {
+    reply_with_error ("%s", err);
+    free(out);
+    return NULL;
+  }
+
+  lines = split_lines (out);
+  if (!lines)
+    return NULL;
+
+  size_t nlines = count_strings (lines);
+  for (i = 0; i < nlines; i++)
+    if (strstr(lines[i], "\tdevid"))
+        nr_filesystem_show++;
+
+  guestfs_int_btrfsfsshow_list *ret = NULL;
+  ret = malloc(sizeof *ret);
+  if (ret == NULL) {
+    reply_with_perror ("malloc");
+    return NULL;
+  }
+
+  /* No btrfs fs found, return directly */
+  if (nr_filesystem_show == 0) {
+    memset (ret, 0, sizeof(*ret));
+    return ret;
+  }
+
+  ret->guestfs_int_btrfsfsshow_list_len = nr_filesystem_show;
+  ret->guestfs_int_btrfsfsshow_list_val +    calloc (nr_filesystem_show,
sizeof (struct guestfs_int_btrfsfsshow));
+  if (ret->guestfs_int_btrfsfsshow_list_val == NULL) {
+    reply_with_perror ("calloc");
+    goto error;
+  }
+
+  /* Output of `btrfs filesystem show' is like:
+   *
+   * Label: none uuid: e34452fa-5444-4e7b-9faa-c26fa1b83686
+   * Total devices 1 FS bytes used 176.00KiB
+   * devid 1 size 1.00GiB used 236.00MiB path /dev/sda6
+   *
+   * Label: none uuid: 9cdb73b9-d5b9-46c4-9395-25a952e7922a
+   * Total devices 5 FS bytes used 112.00KiB
+   * devid 1 size 10.00GiB used 1.02GiB path /dev/sdb
+   * devid 2 size 10.00GiB used 2.00GiB path /dev/sdd
+   * devid 3 size 10.00GiB used 2.00GiB path /dev/sde
+   *
+   * Btrfs v3.18.2
+   *
+   *
+   * If no btrfs device found, output is like:
+   *
+   * Btrfs v3.18.2
+   *
+   */
+  if (nlines < 1) {
+      reply_with_perror ("No filesystem show output");
+      return NULL;
+  }
+
+  char *p, *p1;
+  size_t k;
+
+  for (i = 0, k = 0; i < nlines - 2; i++) {
+    char *line = lines[i];
+
+    struct guestfs_int_btrfsfsshow *this +     
&ret->guestfs_int_btrfsfsshow_list_val[k];
+    if (STRPREFIX(line, "Label")) {
+      if ((p = strstr (line, "uuid")) == NULL) {
+        reply_with_error ("No uuid field found");
+        return NULL;
+      }
+      p1 = lines[i] + strlen("Label: ");
+      if ((p - p1) < 1) {
+        reply_with_error ("No Label field found");
+        return NULL;
+      }
+
+      if (label_tmp) {
+        free (label_tmp);
+        label_tmp = NULL;
+      }
+      if (uuid_tmp) {
+        free (uuid_tmp);
+        uuid_tmp = NULL;
+      }
+
+      p += strlen ("uuid: ");
+      uuid_tmp = strdup(p);
+      p -= strlen ("uuid: ");
+      *p = '\0';
+      label_tmp = strdup(p1);
+      continue;
+    }
+
+    if (STRPREFIX (line, "\tdevid")) {
+      if ((this->btrfsfsshow_uuid = strdup (uuid_tmp)) == NULL)
+        goto error;
+      if ((this->btrfsfsshow_label = strdup (label_tmp)) == NULL)
+        goto error;
+
+      if ((p = strstr (lines[i], "path ")) == NULL) {
+        reply_with_error ("No path field found");
+        goto error;
+      }
+      p += strlen ("path ");
+      if ((this->btrfsfsshow_device = strdup (p)) == NULL)
+        goto error;
+      if (sscanf (line, "\tdevid %4d size %9s used %9s path",
+               &this->btrfsfsshow_devid,
+               this->btrfsfsshow_total_bytes,
+               this->btrfsfsshow_bytes_used) != 3) {
+        reply_with_error ("cannot parse output of filesystem show command:
%s", line);
+        goto error;
+      }
+      k++;
+    }
+  }
+
+  if (label_tmp)
+    free (label_tmp);
+  if (uuid_tmp)
+    free (uuid_tmp);
+  free (lines);
+
+  return ret;
+
+error:
+  free_stringslen (lines, nlines);
+  if (label_tmp)
+    free (label_tmp);
+  if (uuid_tmp)
+    free (uuid_tmp);
+
+  for (i = 0; i < nr_filesystem_show; i++) {
+    struct guestfs_int_btrfsfsshow *this_new +     
&ret->guestfs_int_btrfsfsshow_list_val[i];
+
+    if (this_new->btrfsfsshow_label)
+      free (this_new->btrfsfsshow_label);
+    if (this_new->btrfsfsshow_uuid)
+      free (this_new->btrfsfsshow_uuid);
+    if (this_new->btrfsfsshow_device)
+      free (this_new->btrfsfsshow_device);
+  }
+
+  if (ret)
+    free (ret->guestfs_int_btrfsfsshow_list_val);
+  free (ret);
+
+  return NULL;
+}
diff --git a/generator/actions.ml b/generator/actions.ml
index 7252295..26f9083 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -12593,6 +12593,25 @@ numbered C<partnum> on device C<device>.
 
 It returns C<primary>, C<logical>, or C<extended>." };
 
+  { defaults with
+    name = "btrfs_filesystem_show_all"; added = (1, 29, 46);
+    style = RStructList ("fsshow", "btrfsfsshow"), [], [];
+    (*style = RString "output", [], [OString "device"];*)
+    proc_nr = Some 455;
+    tests = [
+      InitPartition, Always, TestRun (
+        [["part_init"; "/dev/sda"; "mbr"];
+         ["part_add"; "/dev/sda"; "p";
"64"; "204799"];
+         ["part_add"; "/dev/sda"; "p";
"204800"; "-64"];
+         ["mkfs_btrfs"; "/dev/sda1 /dev/sda2";
""; ""; "NOARG"; ""; "NOARG";
"NOARG"; ""; ""];
+         ["mkfs_btrfs"; "/dev/sdb"; "";
""; "NOARG"; ""; "NOARG";
"NOARG"; ""; ""];
+         ["btrfs_filesystem_show_all"]]), [];
+    ];
+    optional = Some "btrfs"; camel_name =
"BTRFSFilesystemShowAll";
+    shortdesc = "show all devices run btrfs filesystem with some
additional info";
+    longdesc = "\
+This show all devices run btrfs filesystem with some additional info." };
+
 ]
 
 (* Non-API meta-commands available only in guestfish.
diff --git a/generator/structs.ml b/generator/structs.ml
index ea110a1..80f03ae 100644
--- a/generator/structs.ml
+++ b/generator/structs.ml
@@ -374,6 +374,19 @@ let structs = [
     ];
     s_camel_name = "BTRFSScrub" };
 
+  (* btrfs filesystem show output *)
+  { defaults with
+    s_name = "btrfsfsshow";
+    s_cols = [
+      "btrfsfsshow_label", FString;
+      "btrfsfsshow_uuid", FString;
+      "btrfsfsshow_devid", FUInt32;
+      "btrfsfsshow_total_bytes", FUUID;
+      "btrfsfsshow_bytes_used", FUUID;
+      "btrfsfsshow_device", FString;
+    ];
+    s_camel_name = "BTRFSFilesystemShowAll" };
+
   (* XFS info descriptor. *)
   { defaults with
     s_name = "xfsinfo";
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index 515f19a..4930863 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-454
+455
-- 
2.1.0

Pino Toscano

2015-Jun-11 09:36 UTC

head link

Re: [Libguestfs] [PATCH] New API: btrfs_filesystem_show_all

Hi,

On Thursday 11 June 2015 12:24:18 Chen Hanxiao wrote:> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> ---
>  daemon/btrfs.c       | 176
+++++++++++++++++++++++++++++++++++++++++++++++++++
>  generator/actions.ml |  19 ++++++
>  generator/structs.ml |  13 ++++
>  src/MAX_PROC_NR      |   2 +-
>  4 files changed, 209 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 39392f7..09f7615 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -2083,3 +2083,179 @@ do_btrfs_image (char *const *sources, const char
*image,
>  
>    return 0;
>  }
> +
> +guestfs_int_btrfsfsshow_list *
> +do_btrfs_filesystem_show_all (void)
Why the _all suffix? If this is about showing the information of a
device, just use a more clear name than just wrapping the btrfs
command.
> +{
> +  const size_t MAX_ARGS = 64;
> +  const char *argv[MAX_ARGS];
> +  size_t i = 0;
> +  size_t nr_filesystem_show = 0;
> +  CLEANUP_FREE char *err = NULL;
> +  char *out = NULL;
"out" will be leaked, so make it CLEANUP_FREE and don't free it
manually.
> +  char **lines;
"lines" can be leaked and it isn't freed properly (only the array
itself
on success, and array and strings on error).
> +  int r;
> +  /* Info from Label line will be reused for the following devid line
> +   * make two temp space to hold them */
> +  char *label_tmp = NULL, *uuid_tmp = NULL;
> +
> +  ADD_ARG (argv, i, str_btrfs);
> +  ADD_ARG (argv, i, "filesystem");
> +  ADD_ARG (argv, i, "show");
> +  ADD_ARG (argv, i, NULL);
> +
> +  r = commandv (&out, &err, argv);
> +  if (r == -1) {
> +    reply_with_error ("%s", err);
> +    free(out);
> +    return NULL;
> +  }
> +
> +  lines = split_lines (out);
> +  if (!lines)
> +    return NULL;
> +
> +  size_t nlines = count_strings (lines);
> +  for (i = 0; i < nlines; i++)
> +    if (strstr(lines[i], "\tdevid"))
> +        nr_filesystem_show++;
The logic here should match what is being done later, i.e. using
STRPREFIX.
> +  guestfs_int_btrfsfsshow_list *ret = NULL;
> +  ret = malloc(sizeof *ret);
Niptick: remember spaces between function name and bracket (also in
other parts of this patch).
> +  if (ret == NULL) {
> +    reply_with_perror ("malloc");
> +    return NULL;
> +  }
> +
> +  /* No btrfs fs found, return directly */
> +  if (nr_filesystem_show == 0) {
> +    memset (ret, 0, sizeof(*ret));
> +    return ret;
> +  }
> +
> +  ret->guestfs_int_btrfsfsshow_list_len = nr_filesystem_show;
> +  ret->guestfs_int_btrfsfsshow_list_val > +    calloc
(nr_filesystem_show, sizeof (struct guestfs_int_btrfsfsshow));
> +  if (ret->guestfs_int_btrfsfsshow_list_val == NULL) {
> +    reply_with_perror ("calloc");
> +    goto error;
> +  }
> +
> +  /* Output of `btrfs filesystem show' is like:
> +   *
> +   * Label: none uuid: e34452fa-5444-4e7b-9faa-c26fa1b83686
> +   * Total devices 1 FS bytes used 176.00KiB
> +   * devid 1 size 1.00GiB used 236.00MiB path /dev/sda6
> +   *
> +   * Label: none uuid: 9cdb73b9-d5b9-46c4-9395-25a952e7922a
> +   * Total devices 5 FS bytes used 112.00KiB
> +   * devid 1 size 10.00GiB used 1.02GiB path /dev/sdb
> +   * devid 2 size 10.00GiB used 2.00GiB path /dev/sdd
> +   * devid 3 size 10.00GiB used 2.00GiB path /dev/sde
> +   *
> +   * Btrfs v3.18.2
> +   *
> +   *
> +   * If no btrfs device found, output is like:
> +   *
> +   * Btrfs v3.18.2
> +   *
> +   */
> +  if (nlines < 1) {
> +      reply_with_perror ("No filesystem show output");
> +      return NULL;
> +  }
> +
> +  char *p, *p1;
> +  size_t k;
> +
> +  for (i = 0, k = 0; i < nlines - 2; i++) {
Instead of arbitrarily ignoring the last two lines (which could change
in newer versions of btrfs-tools), would the code just ignore them?
> +    char *line = lines[i];
> +
> +    struct guestfs_int_btrfsfsshow *this > +     
&ret->guestfs_int_btrfsfsshow_list_val[k];
> +    if (STRPREFIX(line, "Label")) {
> +      if ((p = strstr (line, "uuid")) == NULL) {
> +        reply_with_error ("No uuid field found");
> +        return NULL;
> +      }
> +      p1 = lines[i] + strlen("Label: ");
> +      if ((p - p1) < 1) {
> +        reply_with_error ("No Label field found");
> +        return NULL;
> +      }
> +
> +      if (label_tmp) {
> +        free (label_tmp);
> +        label_tmp = NULL;
> +      }
free (NULL) is a no-op defined by POSIX, so things like

 if (foo)
   free (foo)

can be simplified to just free (foo) (also in other parts of this
patch).
> +      if (uuid_tmp) {
> +        free (uuid_tmp);
> +        uuid_tmp = NULL;
> +      }
> +
> +      p += strlen ("uuid: ");
> +      uuid_tmp = strdup(p);
Missing check of the strdup return value (also in other parts of this
patch).
> +      p -= strlen ("uuid: ");
> +      *p = '\0';
> +      label_tmp = strdup(p1);
> +      continue;
> +    }
> +
> +    if (STRPREFIX (line, "\tdevid")) {
> +      if ((this->btrfsfsshow_uuid = strdup (uuid_tmp)) == NULL)
Please don't mix statements and checks, so:
  x = strdup (s);
  if (x == NULL) ...
> +        goto error;
> +      if ((this->btrfsfsshow_label = strdup (label_tmp)) == NULL)
> +        goto error;
> +
> +      if ((p = strstr (lines[i], "path ")) == NULL) {
"line" is already set as lines[i], isn't it?
> +        reply_with_error ("No path field found");
> +        goto error;
> +      }
> +      p += strlen ("path ");
> +      if ((this->btrfsfsshow_device = strdup (p)) == NULL)
> +        goto error;
> +      if (sscanf (line, "\tdevid %4d size %9s used %9s path",
> +               &this->btrfsfsshow_devid,
> +               this->btrfsfsshow_total_bytes,
> +               this->btrfsfsshow_bytes_used) != 3) {
Why not use this sscanf to also take the device, instead of looking for
it manually using strstr?
> +        reply_with_error ("cannot parse output of filesystem show
command: %s", line);
> +        goto error;
> +      }
> +      k++;
> +    }
> +  }
> +
> +  if (label_tmp)
> +    free (label_tmp);
> +  if (uuid_tmp)
> +    free (uuid_tmp);
> +  free (lines);
This is repeated twice, so maybe make these variables as CLEANUP_FREE,
taking care of properly resetting them to NULL when freeing them.
> +
> +  return ret;
> +
> +error:
> +  free_stringslen (lines, nlines);
As noted above, just make "lines" as CLEANUP_FREE_STRING_LIST.

Also, it seems this wrong handling of the return value of split_lines
has been done in other btrfs implementations:
- do_btrfs_subvolume_list
- do_btrfs_qgroup_show
- do_btrfs_balance_status
- do_btrfs_scrub_status
> +  if (label_tmp)
> +    free (label_tmp);
> +  if (uuid_tmp)
> +    free (uuid_tmp);
> +
> +  for (i = 0; i < nr_filesystem_show; i++) {
> +    struct guestfs_int_btrfsfsshow *this_new > +     
&ret->guestfs_int_btrfsfsshow_list_val[i];
> +
> +    if (this_new->btrfsfsshow_label)
> +      free (this_new->btrfsfsshow_label);
> +    if (this_new->btrfsfsshow_uuid)
> +      free (this_new->btrfsfsshow_uuid);
> +    if (this_new->btrfsfsshow_device)
> +      free (this_new->btrfsfsshow_device);
> +  }
> +
> +  if (ret)
> +    free (ret->guestfs_int_btrfsfsshow_list_val);
> +  free (ret);
> +
> +  return NULL;
> +}
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 7252295..26f9083 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -12593,6 +12593,25 @@ numbered C<partnum> on device
C<device>.
>  
>  It returns C<primary>, C<logical>, or C<extended>."
};
>  
> +  { defaults with
> +    name = "btrfs_filesystem_show_all"; added = (1, 29, 46);
> +    style = RStructList ("fsshow", "btrfsfsshow"), [],
[];
> +    (*style = RString "output", [], [OString
"device"];*)
> +    proc_nr = Some 455;
> +    tests = [
> +      InitPartition, Always, TestRun (
> +        [["part_init"; "/dev/sda"; "mbr"];
> +         ["part_add"; "/dev/sda"; "p";
"64"; "204799"];
> +         ["part_add"; "/dev/sda"; "p";
"204800"; "-64"];
> +         ["mkfs_btrfs"; "/dev/sda1 /dev/sda2";
""; ""; "NOARG"; ""; "NOARG";
"NOARG"; ""; ""];
> +         ["mkfs_btrfs"; "/dev/sdb"; "";
""; "NOARG"; ""; "NOARG";
"NOARG"; ""; ""];
> +         ["btrfs_filesystem_show_all"]]), [];
IMHO a shell (or perl, since it's a struct) test checking the actual
return values would be better.
> +    ];
> +    optional = Some "btrfs"; camel_name =
"BTRFSFilesystemShowAll";
> +    shortdesc = "show all devices run btrfs filesystem with some
additional info";
This description bit (which is repeated also as longdesc) is slightly
cryptic.
> +    longdesc = "\
> +This show all devices run btrfs filesystem with some additional
info." };
> +
>  ]
>  
>  (* Non-API meta-commands available only in guestfish.
> diff --git a/generator/structs.ml b/generator/structs.ml
> index ea110a1..80f03ae 100644
> --- a/generator/structs.ml
> +++ b/generator/structs.ml
> @@ -374,6 +374,19 @@ let structs = [
>      ];
>      s_camel_name = "BTRFSScrub" };
>  
> +  (* btrfs filesystem show output *)
> +  { defaults with
> +    s_name = "btrfsfsshow";
> +    s_cols = [
> +      "btrfsfsshow_label", FString;
> +      "btrfsfsshow_uuid", FString;
> +      "btrfsfsshow_devid", FUInt32;
> +      "btrfsfsshow_total_bytes", FUUID;
> +      "btrfsfsshow_bytes_used", FUUID;
Surely bytes are not an UUID? There's FBytes, so you need to convert
the size strings to bytes.
> +      "btrfsfsshow_device", FString;
> +    ];
> +    s_camel_name = "BTRFSFilesystemShowAll" };
The return value of this API is IMHO confusing. It is a list of
elements, which have duplicated data (like the label, uuid, and device),
so you always get the information of all the btrfs devices and you need
to filter out the devices you are interested in.

I'd say that this API should take a device parameter, just like the rest
of the btrfs-related (and not) APIs, and return all the information
about that only. This would drop the need for the "device" field.

Also, aren't label and uuid available using existing APIs already?

-- 
Pino Toscano

Chen, Hanxiao

2015-Jun-11 10:37 UTC

head link

Re: [Libguestfs] [PATCH] New API: btrfs_filesystem_show_all

Hi,
> -----Original Message-----
> From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com]
On
> Behalf Of Pino Toscano
> Sent: Thursday, June 11, 2015 5:37 PM
> To: libguestfs@redhat.com
> Subject: Re: [Libguestfs] [PATCH] New API: btrfs_filesystem_show_all
> 
> Hi,
> 
> On Thursday 11 June 2015 12:24:18 Chen Hanxiao wrote:
> > Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> > ---
> >  daemon/btrfs.c       | 176
+++++++++++++++++++++++++++++++++++++++++++++++++++
> >  generator/actions.ml |  19 ++++++
> >  generator/structs.ml |  13 ++++
> >  src/MAX_PROC_NR      |   2 +-
> >  4 files changed, 209 insertions(+), 1 deletion(-)
> >
> > diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> > index 39392f7..09f7615 100644
> > --- a/daemon/btrfs.c
> > +++ b/daemon/btrfs.c
> > @@ -2083,3 +2083,179 @@ do_btrfs_image (char *const *sources, const
char *image,
> >
> >    return 0;
> >  }
> > +
> > +guestfs_int_btrfsfsshow_list *
> > +do_btrfs_filesystem_show_all (void)
> 
> Why the _all suffix? If this is about showing the information of a
> device, just use a more clear name than just wrapping the btrfs
> command.
There is another function which could specify device,
but as we discussed before, thought that scenario did not bring some benefit
So I posted a _all API.
> 
> > +{
> > +  const size_t MAX_ARGS = 64;
> > +  const char *argv[MAX_ARGS];
> > +  size_t i = 0;
> > +  size_t nr_filesystem_show = 0;
> > +  CLEANUP_FREE char *err = NULL;
> > +  char *out = NULL;
> 
> "out" will be leaked, so make it CLEANUP_FREE and don't free
it
> manually.
> 
OK.
> > +  char **lines;
> 
> "lines" can be leaked and it isn't freed properly (only the
array itself
> on success, and array and strings on error).
> 
> > +  int r;
> > +  /* Info from Label line will be reused for the following devid line
> > +   * make two temp space to hold them */
> > +  char *label_tmp = NULL, *uuid_tmp = NULL;
> > +
> > +  ADD_ARG (argv, i, str_btrfs);
> > +  ADD_ARG (argv, i, "filesystem");
> > +  ADD_ARG (argv, i, "show");
> > +  ADD_ARG (argv, i, NULL);
> > +
> > +  r = commandv (&out, &err, argv);
> > +  if (r == -1) {
> > +    reply_with_error ("%s", err);
> > +    free(out);
> > +    return NULL;
> > +  }
> > +
> > +  lines = split_lines (out);
> > +  if (!lines)
> > +    return NULL;
> > +
> > +  size_t nlines = count_strings (lines);
> > +  for (i = 0; i < nlines; i++)
> > +    if (strstr(lines[i], "\tdevid"))
> > +        nr_filesystem_show++;
> 
> The logic here should match what is being done later, i.e. using
> STRPREFIX.
> 
> > +  guestfs_int_btrfsfsshow_list *ret = NULL;
> > +  ret = malloc(sizeof *ret);
> 
> Niptick: remember spaces between function name and bracket (also in
> other parts of this patch).
> 
> > +  if (ret == NULL) {
> > +    reply_with_perror ("malloc");
> > +    return NULL;
> > +  }
> > +
> > +  /* No btrfs fs found, return directly */
> > +  if (nr_filesystem_show == 0) {
> > +    memset (ret, 0, sizeof(*ret));
> > +    return ret;
> > +  }
> > +
> > +  ret->guestfs_int_btrfsfsshow_list_len = nr_filesystem_show;
> > +  ret->guestfs_int_btrfsfsshow_list_val > > +    calloc
(nr_filesystem_show, sizeof (struct guestfs_int_btrfsfsshow));
> > +  if (ret->guestfs_int_btrfsfsshow_list_val == NULL) {
> > +    reply_with_perror ("calloc");
> > +    goto error;
> > +  }
> > +
> > +  /* Output of `btrfs filesystem show' is like:
> > +   *
> > +   * Label: none uuid: e34452fa-5444-4e7b-9faa-c26fa1b83686
> > +   * Total devices 1 FS bytes used 176.00KiB
> > +   * devid 1 size 1.00GiB used 236.00MiB path /dev/sda6
> > +   *
> > +   * Label: none uuid: 9cdb73b9-d5b9-46c4-9395-25a952e7922a
> > +   * Total devices 5 FS bytes used 112.00KiB
> > +   * devid 1 size 10.00GiB used 1.02GiB path /dev/sdb
> > +   * devid 2 size 10.00GiB used 2.00GiB path /dev/sdd
> > +   * devid 3 size 10.00GiB used 2.00GiB path /dev/sde
> > +   *
> > +   * Btrfs v3.18.2
> > +   *
> > +   *
> > +   * If no btrfs device found, output is like:
> > +   *
> > +   * Btrfs v3.18.2
> > +   *
> > +   */
> > +  if (nlines < 1) {
> > +      reply_with_perror ("No filesystem show output");
> > +      return NULL;
> > +  }
> > +
> > +  char *p, *p1;
> > +  size_t k;
> > +
> > +  for (i = 0, k = 0; i < nlines - 2; i++) {
> 
> Instead of arbitrarily ignoring the last two lines (which could change
> in newer versions of btrfs-tools), would the code just ignore them?
Yes, current code could do this.
> 
> > +    char *line = lines[i];
> > +
> > +    struct guestfs_int_btrfsfsshow *this > > +     
&ret->guestfs_int_btrfsfsshow_list_val[k];
> > +    if (STRPREFIX(line, "Label")) {
> > +      if ((p = strstr (line, "uuid")) == NULL) {
> > +        reply_with_error ("No uuid field found");
> > +        return NULL;
> > +      }
> > +      p1 = lines[i] + strlen("Label: ");
> > +      if ((p - p1) < 1) {
> > +        reply_with_error ("No Label field found");
> > +        return NULL;
> > +      }
> > +
> > +      if (label_tmp) {
> > +        free (label_tmp);
> > +        label_tmp = NULL;
> > +      }
> 
> free (NULL) is a no-op defined by POSIX, so things like
> 
>  if (foo)
>    free (foo)
> 
> can be simplified to just free (foo) (also in other parts of this
> patch).
> 
Fine.
> > +      if (uuid_tmp) {
> > +        free (uuid_tmp);
> > +        uuid_tmp = NULL;
> > +      }
> > +
> > +      p += strlen ("uuid: ");
> > +      uuid_tmp = strdup(p);
> 
> Missing check of the strdup return value (also in other parts of this
> patch).
> 
Oops..
> > +      p -= strlen ("uuid: ");
> > +      *p = '\0';
> > +      label_tmp = strdup(p1);
> > +      continue;
> > +    }
> > +
> > +    if (STRPREFIX (line, "\tdevid")) {
> > +      if ((this->btrfsfsshow_uuid = strdup (uuid_tmp)) == NULL)
> 
> Please don't mix statements and checks, so:
>   x = strdup (s);
>   if (x == NULL) ...
> 
OK.
> > +        goto error;
> > +      if ((this->btrfsfsshow_label = strdup (label_tmp)) == NULL)
> > +        goto error;
> > +
> > +      if ((p = strstr (lines[i], "path ")) == NULL) {
> 
> "line" is already set as lines[i], isn't it?
> 
Fine.
> > +        reply_with_error ("No path field found");
> > +        goto error;
> > +      }
> > +      p += strlen ("path ");
> > +      if ((this->btrfsfsshow_device = strdup (p)) == NULL)
> > +        goto error;
> > +      if (sscanf (line, "\tdevid %4d size %9s used %9s
path",
> > +               &this->btrfsfsshow_devid,
> > +               this->btrfsfsshow_total_bytes,
> > +               this->btrfsfsshow_bytes_used) != 3) {
> 
> Why not use this sscanf to also take the device, instead of looking for
> it manually using strstr?
If sscanf for device, we still need to calculate the length of 'device'
for malloc.
So strdup would be easier to use.
> 
> > +        reply_with_error ("cannot parse output of filesystem
show command: %s",
> line);
> > +        goto error;
> > +      }
> > +      k++;
> > +    }
> > +  }
> > +
> > +  if (label_tmp)
> > +    free (label_tmp);
> > +  if (uuid_tmp)
> > +    free (uuid_tmp);
> > +  free (lines);
> 
> This is repeated twice, so maybe make these variables as CLEANUP_FREE,
> taking care of properly resetting them to NULL when freeing them.
> 
> > +
> > +  return ret;
> > +
> > +error:
> > +  free_stringslen (lines, nlines);
> 
> As noted above, just make "lines" as CLEANUP_FREE_STRING_LIST.
> 
> Also, it seems this wrong handling of the return value of split_lines
> has been done in other btrfs implementations:
> - do_btrfs_subvolume_list
> - do_btrfs_qgroup_show
> - do_btrfs_balance_status
> - do_btrfs_scrub_status
Yes, I'll send a patch to fix them later.
> 
> > +  if (label_tmp)
> > +    free (label_tmp);
> > +  if (uuid_tmp)
> > +    free (uuid_tmp);
> > +
> > +  for (i = 0; i < nr_filesystem_show; i++) {
> > +    struct guestfs_int_btrfsfsshow *this_new > > +     
&ret->guestfs_int_btrfsfsshow_list_val[i];
> > +
> > +    if (this_new->btrfsfsshow_label)
> > +      free (this_new->btrfsfsshow_label);
> > +    if (this_new->btrfsfsshow_uuid)
> > +      free (this_new->btrfsfsshow_uuid);
> > +    if (this_new->btrfsfsshow_device)
> > +      free (this_new->btrfsfsshow_device);
> > +  }
> > +
> > +  if (ret)
> > +    free (ret->guestfs_int_btrfsfsshow_list_val);
> > +  free (ret);
> > +
> > +  return NULL;
> > +}
> > diff --git a/generator/actions.ml b/generator/actions.ml
> > index 7252295..26f9083 100644
> > --- a/generator/actions.ml
> > +++ b/generator/actions.ml
> > @@ -12593,6 +12593,25 @@ numbered C<partnum> on device
C<device>.
> >
> >  It returns C<primary>, C<logical>, or
C<extended>." };
> >
> > +  { defaults with
> > +    name = "btrfs_filesystem_show_all"; added = (1, 29,
46);
> > +    style = RStructList ("fsshow",
"btrfsfsshow"), [], [];
> > +    (*style = RString "output", [], [OString
"device"];*)
> > +    proc_nr = Some 455;
> > +    tests = [
> > +      InitPartition, Always, TestRun (
> > +        [["part_init"; "/dev/sda";
"mbr"];
> > +         ["part_add"; "/dev/sda"; "p";
"64"; "204799"];
> > +         ["part_add"; "/dev/sda"; "p";
"204800"; "-64"];
> > +         ["mkfs_btrfs"; "/dev/sda1 /dev/sda2";
""; ""; "NOARG"; ""; "NOARG";
> "NOARG"; ""; ""];
> > +         ["mkfs_btrfs"; "/dev/sdb"; "";
""; "NOARG"; ""; "NOARG";
"NOARG"; "";
> ""];
> > +         ["btrfs_filesystem_show_all"]]), [];
> 
> IMHO a shell (or perl, since it's a struct) test checking the actual
> return values would be better.
>
> > +    ];
> > +    optional = Some "btrfs"; camel_name =
"BTRFSFilesystemShowAll";
> > +    shortdesc = "show all devices run btrfs filesystem with some
additional info";
> 
> This description bit (which is repeated also as longdesc) is slightly
> cryptic.
> 
Will improve.
> > +    longdesc = "\
> > +This show all devices run btrfs filesystem with some additional
info." };
> > +
> >  ]
> >
> >  (* Non-API meta-commands available only in guestfish.
> > diff --git a/generator/structs.ml b/generator/structs.ml
> > index ea110a1..80f03ae 100644
> > --- a/generator/structs.ml
> > +++ b/generator/structs.ml
> > @@ -374,6 +374,19 @@ let structs = [
> >      ];
> >      s_camel_name = "BTRFSScrub" };
> >
> > +  (* btrfs filesystem show output *)
> > +  { defaults with
> > +    s_name = "btrfsfsshow";
> > +    s_cols = [
> > +      "btrfsfsshow_label", FString;
> > +      "btrfsfsshow_uuid", FString;
> > +      "btrfsfsshow_devid", FUInt32;
> > +      "btrfsfsshow_total_bytes", FUUID;
> > +      "btrfsfsshow_bytes_used", FUUID;
> 
> Surely bytes are not an UUID? There's FBytes, so you need to convert
> the size strings to bytes.
But filesystem show could not output raw data...
> 
> > +      "btrfsfsshow_device", FString;
> > +    ];
> > +    s_camel_name = "BTRFSFilesystemShowAll" };
> 
> The return value of this API is IMHO confusing. It is a list of
> elements, which have duplicated data (like the label, uuid, and device),
> so you always get the information of all the btrfs devices and you need
> to filter out the devices you are interested in.
> 
> I'd say that this API should take a device parameter, just like the
rest
> of the btrfs-related (and not) APIs, and return all the information
> about that only. This would drop the need for the "device" field.
> 
> Also, aren't label and uuid available using existing APIs already?
Although we had some kind of this API, this API could combine
them together and bring us convenient. 

Pls check:
https://www.redhat.com/archives/libguestfs/2015-March/msg00187.html

Regards,
- Chen
> 
> --
> Pino Toscano
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

Possibly Parallel Threads

Search for more apparently analagous threads

Libguestfs - Jun 2015 - Re: [PATCH] New API: btrfs_filesystem_show_all

[Libguestfs] [PATCH] New API: btrfs_filesystem_show_all

Re: [Libguestfs] [PATCH] New API: btrfs_filesystem_show_all

Re: [Libguestfs] [PATCH] New API: btrfs_filesystem_show_all

Possibly Parallel Threads