[Libguestfs] [PATCH] New API: btrfs_filesystem_show_all

[Pino Toscano]

On Thursday 11 June 2015 10:37:18 Chen, Hanxiao wrote:
> 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.

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