[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/7] conf: add features to volume target XML



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 @@
>              &lt;mode&gt;0744&lt;/mode&gt;
>              &lt;label&gt;virt_image_t&lt;/label&gt;
>            &lt;/permissions&gt;
> +          &lt;compat&gt;1.1&lt;/compat&gt;
> +          &lt;features&gt;
> +            &lt;lazy_refcounts/&gt;
> +          &lt;/features&gt;
>          &lt;/target&gt;</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>&lt;lazy_refcounts/&gt;</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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]