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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list