11:24 p.m.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
4:36 a.m.
Hi,
On Thursday 11 June 2015 12:24:18 Chen Hanxiao wrote:
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
5:37 a.m.
Hi,
-----Original Message-----
From: libguestfs-bounces(a)redhat.com [mailto:libguestfs-bounces@redhat.com] On
Behalf Of Pino Toscano
Sent: Thursday, June 11, 2015 5:37 PM
To: libguestfs(a)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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
6:59 a.m.
On Thursday 11 June 2015 10:37:18 Chen, Hanxiao wrote:
Hi,
> -----Original Message-----
> From: libguestfs-bounces(a)redhat.com [mailto:libguestfs-bounces@redhat.com] On
> Behalf Of Pino Toscano
> Sent: Thursday, June 11, 2015 5:37 PM
> To: libguestfs(a)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(a)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.
The discussion was whether the API was useful at all, or it is just
something that advanced users will need when recovering/manipulating
images using virt-rescue.
> > + 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.
I was not referring to the usage of strdup, but to the manual search of
"path " using strstr. Also, you can use the allocation features of
sscanf, e.g.:
char *p;
sscanf("somestring", "%ms", &p);
(this is also documented in sscanf(3).)
> > + 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...
Then you need to do the conversion on your own.
> > + "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.
I don't understand what you mean here. Your proposed API for a single
filesystem_show was returning data unformatted, unparsed. We have
already APIs to list all the available filesystems, and most of the
other btrfs APIs act directly on a given device/filesystem.
Thanks,
--
Pino Toscano
3376
days inactive
3376
days old
3 comments
3 participants
participants (3)
-
Chen Hanxiao -
Chen, Hanxiao
-
Pino Toscano