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

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.

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