On 06/10/2013 02:10 PM, Ján Tomko wrote: > Add <features> and <compat> elements to volume target XML. > > <compat> is a string which for qcow2 represents the QEMU version > it should be compatible with. Valid values are 0.10 and 1.1. > 1.1 is implicit if the <features> element is present, otherwise > qemu-img default is used. 0.10 can be specified to explicitly > create older images after the qemu-img default changes. > Do I understand it correctly that in case there will be next format, we'll use the correct one depending on what features are requested? > <features> contains optional features, so far > <lazy_refcounts/> is available, which enables caching of reference > counters, improving performance for snapshots. > --- > docs/formatstorage.html.in | 19 +++++++ > docs/schemas/Makefile.am | 1 + > docs/schemas/storagefilefeatures.rng | 24 +++++++++ > docs/schemas/storagevol.rng | 7 +++ > libvirt.spec.in | 1 + > mingw-libvirt.spec.in | 2 + > src/conf/storage_conf.c | 74 +++++++++++++++++++++++++++ > src/conf/storage_conf.h | 7 ++- > src/libvirt_private.syms | 2 + > src/storage/storage_backend_fs.c | 10 ++++ > tests/storagevolxml2xmlin/vol-qcow2-1.1.xml | 32 ++++++++++++ > tests/storagevolxml2xmlin/vol-qcow2-lazy.xml | 35 +++++++++++++ > tests/storagevolxml2xmlout/vol-qcow2-1.1.xml | 33 ++++++++++++ > tests/storagevolxml2xmlout/vol-qcow2-lazy.xml | 35 +++++++++++++ > tests/storagevolxml2xmltest.c | 2 + > 15 files changed, 282 insertions(+), 2 deletions(-) > create mode 100644 docs/schemas/storagefilefeatures.rng > create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-1.1.xml > create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-lazy.xml > create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-1.1.xml > create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-lazy.xml > > diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in > index 1a45915..30a07f4 100644 > --- a/docs/formatstorage.html.in > +++ b/docs/formatstorage.html.in > @@ -334,6 +334,10 @@ > <mode>0744</mode> > <label>virt_image_t</label> > </permissions> > + <compat>1.1</compat> > + <features> > + <lazy_refcounts/> > + </features> > </target></pre> > > <dl> > @@ -362,6 +366,21 @@ > contains the MAC (eg SELinux) label string. > <span class="since">Since 0.4.1</span> > </dd> > + <dt><code>compat</code></dt> > + <dd>Specify compatibility level. So far, this is only used for > + <code>type='qcow2'</code> volumes. Valid values are <code>0.10</code> > + and <code>1.1</code> so far, specifying QEMU version the images should > + be compatible with. If the <code>feature</code> element is present, > + 1.1 is used. If omitted, qemu-img default is used. If my previous presumption is correct, then it might be worth putting it here, but leaving it like this until next feature comes is OK too. > + <span class="since">Since 1.0.6</span> s/6/7/ > + </dd> > + <dt><code>features</code></dt> > + <dd>Format-specific features. Only used for <code>qcow2</code> now. > + Valid sub-elements are: > + <code><lazy_refcounts/></code> - allow delayed reference > + counter updates. This would look better if a list were used. > + <span class="since">Since 1.0.6</span> s/6/7/ > + </dd> > </dl> > > <h3><a name="StorageVolBacking">Backing store elements</a></h3> > diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am > index 8da2c67..47d1941 100644 > --- a/docs/schemas/Makefile.am > +++ b/docs/schemas/Makefile.am > @@ -28,6 +28,7 @@ schema_DATA = \ > nwfilter.rng \ > secret.rng \ > storageencryption.rng \ > + storagefilefeatures.rng \ > storagepool.rng \ > storagevol.rng > > diff --git a/docs/schemas/storagefilefeatures.rng b/docs/schemas/storagefilefeatures.rng > new file mode 100644 > index 0000000..96a1829 > --- /dev/null > +++ b/docs/schemas/storagefilefeatures.rng > @@ -0,0 +1,24 @@ > +<?xml version="1.0"?> > +<!-- A Relax NG schema for the libvirt volume features XML format --> > +<grammar xmlns="http://relaxng.org/ns/structure/1.0"; > + datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes";> > + > + <define name='compat'> > + <element name='compat'> > + <data type='string'> > + <param name='pattern'>[^,]+</param> I'd change this to "[0-9]+\.[0-9]+" > + </data> > + </element> > + </define> > + <define name='fileFormatFeatures'> > + <element name='features'> > + <interleave> > + <optional> > + <element name='lazy_refcounts'> > + <empty/> > + </element> > + </optional> > + </interleave> > + </element> > + </define> > +</grammar> [...] > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index c58b728..35ad502 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -97,6 +97,8 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapterType, > > typedef const char *(*virStorageVolFormatToString)(int format); > typedef int (*virStorageVolFormatFromString)(const char *format); > +typedef const char *(*virStorageVolFeatureToString)(int feature); > +typedef int (*virStorageVolFeatureFromString)(const char *feature); > > typedef const char *(*virStoragePoolFormatToString)(int format); > typedef int (*virStoragePoolFormatFromString)(const char *format); > @@ -107,6 +109,9 @@ struct _virStorageVolOptions { > int defaultFormat; > virStorageVolFormatToString formatToString; > virStorageVolFormatFromString formatFromString; > + virStorageVolFeatureToString featureToString; > + virStorageVolFeatureFromString featureFromString; > + int featureLast; Will there be anything else than VIR_STORAGE_FILE_FEATURE_LAST ??? > }; > > /* Flags to indicate mandatory components in the pool source */ [...] > @@ -1335,12 +1353,46 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, > VIR_FREE(format); > } > > + ret->target.compat = virXPathString("string(./target/compat)", ctxt); > + if (ret->target.compat && strchr(ret->target.compat, ',')) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("forbidden characters in 'compat' attribute")); We should check for: - available (known) compat values (very strict, not forward-compatible) - the same pattern as in RelaxNG scheme (strict, forward-compatible, ugly code) - or allow use of any string, but escape it properly when handed to qemu, for example like this: qemu-img create -f qcow2 -o compat=0.10,,preallocation=metadata \ delete.me 1M Formatting 'delete.me', fmt=qcow2 size=1048576 compat='0.10,preallocation=metadata' encryption=off cluster_size=65536 lazy_refcounts=off Invalid compatibility level: '0.10,preallocation=metadata' qemu-img: delete.me: error while creating qcow2: Invalid argument Which would make it forward-compatible, but since we have to make sure we report potential qemu-img errors back properly, I don't see a reason why there should be a problem with that. Personally, I'm for the second option. > + goto cleanup; > + } > + > + if (virXPathNode("./target/features", ctxt) && options->featureLast) { > + if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) > + goto cleanup; > + > + if (!ret->target.compat && VIR_STRDUP(ret->target.compat, "1.1") < 0) > + goto cleanup; > + > + if (!(ret->target.features = virBitmapNew(options->featureLast))) > + goto no_memory; > + Can't wait for Michal's unified OOM reporting to hit upstream, this looks weird (cleanup/no_memory). > + for (i = 0; i < n; i++) { > + int f = options->featureFromString((const char*)nodes[i]->name); > + > + if (f < 0) { > + virReportError(VIR_ERR_XML_ERROR, _("unsupported feature %s"), > + (const char*)nodes[i]->name); > + goto cleanup; > + } > + ignore_value(virBitmapSetBit(ret->target.features, f)); > + } > + VIR_FREE(nodes); > + } > + All the "goto cleanup;" statements in this hunk should be "goto error;". > if (virStorageDefParsePerms(ctxt, &ret->backingStore.perms, > "./backingStore/permissions", > DEFAULT_VOL_PERM_MODE) < 0) > goto error; > > +no_memory: > + virReportOOMError(); > + You probably don't want this to be reported even when everything went OK, you probably wanted this before the "error:" label. > cleanup: > + VIR_FREE(nodes); > VIR_FREE(allocation); > VIR_FREE(capacity); > VIR_FREE(unit); You should also check whether the features specified are correctly supported with the compat level (if specified), so we report nicer error than "internal error". No need to check what qemu-img supports, since we know what compat level is needed for what features. Other than these, this looks fine, but seeing the fixes in another version would be better. Martin -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list