On Wed, Nov 13, 2013 at 11:18:01AM +0000, Daniel P. Berrange wrote:
> On Wed, Nov 13, 2013 at 12:07:43PM +0100, Peter Krempa wrote:
> > Consider the following valid snapshot XML as the <driver> element is
> > allowed to be empty in the domainsnapshot.rng schema:
> >
> > $ cat snap.xml
> > <domainsnapshot>
> > <disks>
> > <disk name='vda' snapshot='external'>
> > <source file='/tmp/foo'/>
> > <driver/>
> > </disk>
> > </disks>
> > </domainsnapshot>
> >
> > produces the following error:
> >
> > $ virsh snapshot-create domain snap.xml
> > error: internal error: unknown disk snapshot driver '(null)'
> >
> > The driver type is parsed as NULL from the XML as the attribute is not
> > present and then directly used to produce the error message.
> >
> > With this patch the attempt to parse the driver type is skipped if not
> > present to avoid changing the schema to forbid the empty driver element.
> > ---
> > src/conf/snapshot_conf.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> > index d8910c9..418987b 100644
> > --- a/src/conf/snapshot_conf.c
> > +++ b/src/conf/snapshot_conf.c
> > @@ -175,15 +175,17 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
> > } else if (!def->format &&
> > xmlStrEqual(cur->name, BAD_CAST "driver")) {
> > char *driver = virXMLPropString(cur, "type");
> > - def->format = virStorageFileFormatTypeFromString(driver);
> > - if (def->format <= 0) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("unknown disk snapshot driver '%s'"),
> > - driver);
> > + if (driver) {
> > + def->format = virStorageFileFormatTypeFromString(driver);
> > + if (def->format <= 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("unknown disk snapshot driver '%s'"),
> > + driver);
> > + VIR_FREE(driver);
> > + goto cleanup;
> > + }
> > VIR_FREE(driver);
> > - goto cleanup;
> > }
> > - VIR_FREE(driver);
> > }
> > }
>
> So IIUC, this allows
>
> <driver type="qemu"/>
> <driver type="qemu" format="qcow2" />
>
> but forbids
>
> <driver format="qcow2" />
>
> The domain XML parser, however, allows all 3 and 'type' will be auto
> determined if omitted. IMHO it would be desirable to be consistent
> between the parsers
Oh, ignore me. I totally mis-read the code - getting confused by the
different variable names used for the same thing.
ACK to the patch.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list