[Libguestfs] [PATCH v2 03/18] New API parameter: Add discard parameter to guestfs_add_drive_opts.

[Richard W.M. Jones]

On Wed, Mar 12, 2014 at 10:45:17AM +0100, Pino Toscano wrote:
> On Tuesday 11 March 2014 23:13:46 Richard W.M. Jones wrote:
> > diff --git a/src/drives.c b/src/drives.c
> > index 2c85b52..68e37f7 100644
> > --- a/src/drives.c
> > +++ b/src/drives.c
> > @@ -115,7 +115,8 @@ static struct drive *
> >  create_drive_file (guestfs_h *g, const char *path,
> >                     bool readonly, const char *format,
> >                     const char *iface, const char *name,
> > -                   const char *disk_label, const char *cachemode)
> > +                   const char *disk_label, const char *cachemode,
> > +                   enum discard discard)
> >  {
> >    struct drive *drv = safe_calloc (g, 1, sizeof *drv);
> > 
> 
> (and others)
> 
> It seems like these functions, albeit internal and private to this
> file, could need some help in avoid the over-long list of parameters,
> which all need to be changed every time there's an argument change
> (just like now).
> 
> I'll do a small refactor.

Yes, there's definitely some refactoring which would help with
parameter passing in this file.

> > --- a/src/guestfs-internal.h
> > +++ b/src/guestfs-internal.h
> > @@ -231,6 +231,12 @@ struct drive_source {
> >    char *secret;
> >  };
> > 
> > +enum discard {
> > +  discard_disable = 0,
> > +  discard_enable = 1,
> > +  discard_besteffort = 2,
> > +};
> 
> I guess the manual numbering is not needed.

I set discard_disable = 0 to be safe (I think the C standard implies
it anyway), because we rely on calloc to set the default value.  I'll
remove the other two in v3 of this series.

> > 
> > -      if (construct_libvirt_xml_disk_driver_qemu (g, xo, "qcow2", "unsafe")
> > +      if (construct_libvirt_xml_disk_driver_qemu (g, NULL, NULL,
> > +                                                  xo, "qcow2", "unsafe",
> > +                                                  discard_disable) == -1)
> >          return -1;
> >      }
> 
> ... and ...
> 
> > @@ -1499,7 +1544,9 @@ construct_libvirt_xml_appliance (guestfs_h *g,
> >        attribute ("bus", "scsi");
> >      } end_element ();
> > 
> > -    if (construct_libvirt_xml_disk_driver_qemu (g, xo, "qcow2", "unsafe") == -1)
> > +    if (construct_libvirt_xml_disk_driver_qemu (g, NULL, NULL, xo,
> > +                                                "qcow2", "unsafe",
> > +                                                discard_disable) == -1) return -1;
> > 
> >      if (construct_libvirt_xml_disk_address (g, xo,
> 
> Even if passing discard_disable makes sure that now "data" and "drv"
> are not used within construct_libvirt_xml_disk_driver_qemu, wouldn't be
> better to pass them anyway if possible, so further code might use them
> (at least "data") safely?

Unfortunately in the second call we don't have 'drv' (because we're
adding the appliance disk which has no associated drv struct).

However I will add an assert & a comment into
construct_libvirt_xml_disk_driver_qemu which should mean we won't
break that function in future.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/