On 11/13/13 12:18, 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" />

I guess it's s/type/name/, s/format/type/ in the domain definition: Thus in the snapshot parser we have only the "type" attribute that is being parsed using virStorageFileFormatTypeFromString(). The schema allows: <source> <driver/> and <source> <driver type='qcow2'/> (or some other format) The first of those two isn't actually any useful as the snapshot creation will fail anyways as it's supported only with qcow2 and similar.

but forbids

<driver format="qcow2" />

<driver type='qcow2'/> is actually allowed and quite useful. We just don't have the "name" attribute here. Is it worth adding it without having it to do anything?

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

We could probably auto-assign qcow2 as the type if not provided to allow creating of the snapshot without the need to do it explicitly.

Daniel

Peter PEter