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

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&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. 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.

> > + 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.

> > + 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.