[libvirt] [glib PATCH] domain config: add API to set the filesystem image format

[Cedric Bosdonnat]

On Mon, 2015-06-15 at 17:12 +0200, Christophe Fergeau wrote:
> Hey,
> 
> On Mon, Jun 15, 2015 at 03:37:12PM +0200, Cédric Bosdonnat wrote:
> > Add the gvir_config_domain_filesys_set_driver_format function to allow
> > setting nbd driver type + image format for containers filesystems.
> > ---
> >  libvirt-gconfig/libvirt-gconfig-domain-filesys.c | 30 +++++++++++++++++++++++-
> >  libvirt-gconfig/libvirt-gconfig-domain-filesys.h |  4 ++++
> >  libvirt-gconfig/libvirt-gconfig.sym              |  5 ++++
> >  3 files changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libvirt-gconfig/libvirt-gconfig-domain-filesys.c b/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
> > index 006a407..fffbe88 100644
> > --- a/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
> > +++ b/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
> > @@ -125,7 +125,9 @@ void gvir_config_domain_filesys_set_driver_type(GVirConfigDomainFilesys *filesys
> >      GVirConfigObject *node;
> >  
> >      g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_FILESYS(filesys));
> > -    node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(filesys), "driver");
> > +    if (!(node = gvir_config_object_get_child(GVIR_CONFIG_OBJECT(filesys), "driver"))) {
> > +        node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(filesys), "driver");
> > +    }
> 
> I believe you could use gvir_config_object_replace_child() here? This
> should be split in a different commit.
> 
> 
> >      g_return_if_fail(GVIR_CONFIG_IS_OBJECT(node));
> >      if (type != GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_DEFAULT)
> >          gvir_config_object_set_attribute_with_type(
> > @@ -137,6 +139,32 @@ void gvir_config_domain_filesys_set_driver_type(GVirConfigDomainFilesys *filesys
> >      g_object_unref(G_OBJECT(node));
> >  }
> >  
> > +void gvir_config_domain_filesys_set_driver_format(GVirConfigDomainFilesys *filesys,
> > +                                                  GVirConfigDomainDiskFormat format)
> > +{
> > +    GVirConfigObject *node;
> > +    GVirConfigDomainFilesysDriverType type = GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_LOOP;
> > +
> > +    g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_FILESYS(filesys));
> > +    if (!(node = gvir_config_object_get_child(GVIR_CONFIG_OBJECT(filesys), "driver"))) {
> > +        node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(filesys), "driver");
> > +    }
> 
> _replace_child() ?
> 
> > +    g_return_if_fail(GVIR_CONFIG_IS_OBJECT(node));
> > +    if (format != GVIR_CONFIG_DOMAIN_DISK_FORMAT_RAW)
> > +        type = GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_NBD;
> > +
> > +    gvir_config_object_set_attribute_with_type(
> > +        node, "type",
> > +        GVIR_CONFIG_TYPE_DOMAIN_FILESYS_DRIVER_TYPE,
> > +        type, NULL);
> > +
> > +    gvir_config_object_set_attribute_with_type(
> > +        node, "format",
> > +        GVIR_CONFIG_TYPE_DOMAIN_DISK_FORMAT,
> > +        format, NULL);
> 
> These 2 calls can probably be grouped in a single one?
> 
> I haven't looked if there are other similar situations in
> libvirt-gconfig, but silently overwriting a preexisting "type" attribute
> with something different when setting the format does not seem very nice
> to the library user.

What should we do in that case? Plainly return an error or just a
warning?

> 
> > +    g_object_unref(G_OBJECT(node));
> > +}
> > +
> >  void gvir_config_domain_filesys_set_source(GVirConfigDomainFilesys *filesys,
> >                                             const char *source)
> >  {
> > diff --git a/libvirt-gconfig/libvirt-gconfig-domain-filesys.h b/libvirt-gconfig/libvirt-gconfig-domain-filesys.h
> > index 4f3973e..18c4069 100644
> > --- a/libvirt-gconfig/libvirt-gconfig-domain-filesys.h
> > +++ b/libvirt-gconfig/libvirt-gconfig-domain-filesys.h
> > @@ -75,6 +75,8 @@ typedef enum {
> >      GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_DEFAULT,
> >      GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_PATH,
> >      GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_HANDLE,
> > +    GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_LOOP,
> > +    GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_NBD,
> >  } GVirConfigDomainFilesysDriverType;
> 
> Different commit?
> 
> Could you add some small test case for the filesys node to
> tests/test/gconfig.c while you are at it?

I'll do it... but it really seems that there are a lot of untested areas
there ;)

--
Cedric