[Libguestfs] [PATCH] New API: btrfs_filesystem_show_all
Chen, Hanxiao
chenhanxiao at cn.fujitsu.com
Thu Jun 11 10:37:18 UTC 2015
Hi,
> -----Original Message-----
> From: libguestfs-bounces at redhat.com [mailto:libguestfs-bounces at redhat.com] On
> Behalf Of Pino Toscano
> Sent: Thursday, June 11, 2015 5:37 PM
> To: libguestfs at 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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
More information about the Libguestfs
mailing list