On Mon, Jun 18, 2012 at 10:21:01AM +0200, Christophe Fergeau wrote:
> On Fri, Jun 15, 2012 at 03:57:21PM +0100, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> >
> > When setting the filesystem source type, the code forgot to
> > update priv->type. Thus when setting the source element,
> > the incorrect attribute was being used.
> > ---
> > .../libvirt-gconfig-domain-device-private.h | 3 +++
> > libvirt-gconfig/libvirt-gconfig-domain-device.c | 2 +-
> > libvirt-gconfig/libvirt-gconfig-domain-filesys.c | 28 ++++++++++++++++++++
> > 3 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device-private.h b/libvirt-gconfig/libvirt-gconfig-domain-device-private.h
> > index f50946a..3ec83c3 100644
> > --- a/libvirt-gconfig/libvirt-gconfig-domain-device-private.h
> > +++ b/libvirt-gconfig/libvirt-gconfig-domain-device-private.h
> > @@ -37,6 +37,9 @@ GVirConfigDomainDevice *
> > gvir_config_domain_disk_new_from_tree(GVirConfigXmlDoc *doc,
> > xmlNodePtr tree);
> > GVirConfigDomainDevice *
> > +gvir_config_domain_filesys_new_from_tree(GVirConfigXmlDoc *doc,
> > + xmlNodePtr tree);
> > +GVirConfigDomainDevice *
> > gvir_config_domain_graphics_new_from_tree(GVirConfigXmlDoc *doc,
> > xmlNodePtr tree);
> > GVirConfigDomainDevice *
> > diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c
> > index 9a7fae5..60d6bcc 100644
> > --- a/libvirt-gconfig/libvirt-gconfig-domain-device.c
> > +++ b/libvirt-gconfig/libvirt-gconfig-domain-device.c
> > @@ -62,7 +62,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc,
> > if (xmlStrEqual(tree->name, (xmlChar*)"disk")) {
> > return gvir_config_domain_disk_new_from_tree(doc, tree);
> > } else if (xmlStrEqual(tree->name, (xmlChar*)"filesystem")) {
> > - type = GVIR_CONFIG_TYPE_DOMAIN_FILESYS;
> > + return gvir_config_domain_filesys_new_from_tree(doc, tree);
> > } else if (xmlStrEqual(tree->name, (xmlChar*)"controller")) {
> > return gvir_config_domain_controller_new_from_tree(doc, tree);
> > } else if (xmlStrEqual(tree->name, (xmlChar*)"lease")) {
> > diff --git a/libvirt-gconfig/libvirt-gconfig-domain-filesys.c b/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
> > index 904a7a3..cc2f604 100644
> > --- a/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
> > +++ b/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
> > @@ -69,6 +69,33 @@ GVirConfigDomainFilesys *gvir_config_domain_filesys_new_from_xml(const gchar *xm
> > return GVIR_CONFIG_DOMAIN_FILESYS(object);
> > }
> >
> > +GVirConfigDomainDevice *
> > +gvir_config_domain_filesys_new_from_tree(GVirConfigXmlDoc *doc,
> > + xmlNodePtr tree)
> > +{
> > + GVirConfigObject *object;
> > + GVirConfigDomainFilesys *filesys;
> > + GVirConfigDomainFilesysType type;
> > + const char *type_str;
> > +
> > + type_str = gvir_config_xml_get_attribute_content(tree, "type");
> > + if (type_str == NULL)
> > + return NULL;
> > +
> > + type = gvir_config_genum_get_value(GVIR_CONFIG_TYPE_DOMAIN_FILESYS_TYPE,
> > + type_str,
> > + GVIR_CONFIG_DOMAIN_FILESYS_FILE);
> > + if (type == -1)
> > + return NULL;
>
> This cannot happen, gvir_config_genum_get_value will always return the
> default value (GVIR_CONFIG_DOMAIN_FILESYS_FILE) if there's a failure.
> Apart from this issue, it looks good.
Hehe, you better tell that to yourself then - I copy+pasted this code
from the libvirt-gconfig-domain-disk.c code which you wrote ;-P
Seriously though, I'll post a patch to fix existing instances of this
flaw.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
