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

-----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&gt; > --- > 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 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs