[libvirt] [PATCH 0/9] Make capacity optional when creating volumes

If the volume has a <backingStore> or we are cloning a volume, we don't need to know the capacity upfront. https://bugzilla.redhat.com/show_bug.cgi?id=958510 Ján Tomko (9): Add flags argument to virStorageVolDefParse* Parse backingStore before capacity in volume XML Allow parsing volumes without specifying the capacity Allow cloning volumes with no capacity specified Fix error messages in virStorageFileGetMetadataFromFD Probe for capacity in virStorageBackendUpdateVolTargetInfo Revert "Restore skipping of setting capacity" Allow omitting volume capacity when backing store is specified Allow creating volumes with a backing store but no capacity src/conf/storage_conf.c | 99 ++++++++++++---------- src/conf/storage_conf.h | 15 +++- src/esx/esx_storage_backend_vmfs.c | 4 +- src/parallels/parallels_storage.c | 6 +- src/phyp/phyp_driver.c | 2 +- src/storage/storage_backend.c | 60 +++++++++---- src/storage/storage_backend.h | 5 +- src/storage/storage_backend_disk.c | 4 +- src/storage/storage_backend_fs.c | 17 ++-- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_rbd.c | 6 ++ src/storage/storage_backend_scsi.c | 2 +- src/storage/storage_backend_sheepdog.c | 6 ++ src/storage/storage_driver.c | 17 +++- src/test/test_driver.c | 6 +- src/util/virstoragefile.c | 4 +- src/vbox/vbox_storage.c | 2 +- tests/storagebackendsheepdogtest.c | 2 +- .../qcow2-nocapacity-convert-prealloc.argv | 4 + tests/storagevolxml2argvdata/qcow2-nocapacity.argv | 5 ++ tests/storagevolxml2argvtest.c | 29 +++++-- .../vol-qcow2-nocapacity-backing.xml | 23 +++++ tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml | 19 +++++ .../storagevolxml2xmlout/vol-qcow2-nocapacity.xml | 21 +++++ tests/storagevolxml2xmltest.c | 15 ++-- 27 files changed, 269 insertions(+), 110 deletions(-) create mode 100644 tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv create mode 100644 tests/storagevolxml2argvdata/qcow2-nocapacity.argv create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-nocapacity-backing.xml create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-nocapacity.xml -- 2.0.5

Allow the callers to pass down libvirt-internal flags. --- src/conf/storage_conf.c | 25 ++++++++++++++++--------- src/conf/storage_conf.h | 9 ++++++--- src/esx/esx_storage_backend_vmfs.c | 4 ++-- src/parallels/parallels_storage.c | 6 +++--- src/phyp/phyp_driver.c | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 6 +++--- src/vbox/vbox_storage.c | 2 +- tests/storagebackendsheepdogtest.c | 2 +- tests/storagevolxml2argvtest.c | 4 ++-- tests/storagevolxml2xmltest.c | 13 ++++++++----- 11 files changed, 45 insertions(+), 32 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e9aaa8a..c8a860b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1245,7 +1245,8 @@ virStorageSize(const char *unit, static virStorageVolDefPtr virStorageVolDefParseXML(virStoragePoolDefPtr pool, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned long flags) { virStorageVolDefPtr ret; virStorageVolOptionsPtr options; @@ -1259,6 +1260,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, size_t i; int n; + virCheckFlags(0, NULL); + options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) return NULL; @@ -1429,7 +1432,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, virStorageVolDefPtr virStorageVolDefParseNode(virStoragePoolDefPtr pool, xmlDocPtr xml, - xmlNodePtr root) + xmlNodePtr root, + unsigned long flags) { xmlXPathContextPtr ctxt = NULL; virStorageVolDefPtr def = NULL; @@ -1449,7 +1453,7 @@ virStorageVolDefParseNode(virStoragePoolDefPtr pool, } ctxt->node = root; - def = virStorageVolDefParseXML(pool, ctxt); + def = virStorageVolDefParseXML(pool, ctxt, flags); cleanup: xmlXPathFreeContext(ctxt); return def; @@ -1458,13 +1462,14 @@ virStorageVolDefParseNode(virStoragePoolDefPtr pool, static virStorageVolDefPtr virStorageVolDefParse(virStoragePoolDefPtr pool, const char *xmlStr, - const char *filename) + const char *filename, + unsigned long flags) { virStorageVolDefPtr ret = NULL; xmlDocPtr xml; if ((xml = virXMLParse(filename, xmlStr, _("(storage_volume_definition)")))) { - ret = virStorageVolDefParseNode(pool, xml, xmlDocGetRootElement(xml)); + ret = virStorageVolDefParseNode(pool, xml, xmlDocGetRootElement(xml), flags); xmlFreeDoc(xml); } @@ -1473,16 +1478,18 @@ virStorageVolDefParse(virStoragePoolDefPtr pool, virStorageVolDefPtr virStorageVolDefParseString(virStoragePoolDefPtr pool, - const char *xmlStr) + const char *xmlStr, + unsigned long flags) { - return virStorageVolDefParse(pool, xmlStr, NULL); + return virStorageVolDefParse(pool, xmlStr, NULL, flags); } virStorageVolDefPtr virStorageVolDefParseFile(virStoragePoolDefPtr pool, - const char *filename) + const char *filename, + unsigned long flags) { - return virStorageVolDefParse(pool, NULL, filename); + return virStorageVolDefParse(pool, NULL, filename, flags); } static void diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 2c9eaed..2162426 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -347,14 +347,17 @@ char *virStoragePoolDefFormat(virStoragePoolDefPtr def); virStorageVolDefPtr virStorageVolDefParseString(virStoragePoolDefPtr pool, - const char *xml); + const char *xml, + unsigned long flags); virStorageVolDefPtr virStorageVolDefParseFile(virStoragePoolDefPtr pool, - const char *filename); + const char *filename, + unsigned long flags); virStorageVolDefPtr virStorageVolDefParseNode(virStoragePoolDefPtr pool, xmlDocPtr xml, - xmlNodePtr root); + xmlNodePtr root, + unsigned long flags); char *virStorageVolDefFormat(virStoragePoolDefPtr pool, virStorageVolDefPtr def); diff --git a/src/esx/esx_storage_backend_vmfs.c b/src/esx/esx_storage_backend_vmfs.c index fa423e9..0dcf419 100644 --- a/src/esx/esx_storage_backend_vmfs.c +++ b/src/esx/esx_storage_backend_vmfs.c @@ -864,7 +864,7 @@ esxStorageVolCreateXML(virStoragePoolPtr pool, } /* Parse config */ - def = virStorageVolDefParseString(&poolDef, xmldesc); + def = virStorageVolDefParseString(&poolDef, xmldesc, 0); if (!def) goto cleanup; @@ -1085,7 +1085,7 @@ esxStorageVolCreateXMLFrom(virStoragePoolPtr pool, goto cleanup; /* Parse config */ - def = virStorageVolDefParseString(&poolDef, xmldesc); + def = virStorageVolDefParseString(&poolDef, xmldesc, 0); if (!def) goto cleanup; diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index d2c5bf2..a6980a4 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -1208,9 +1208,9 @@ parallelsStorageVolDefineXML(virStoragePoolObjPtr pool, char *xml_path = NULL; if (xmlfile) - privvol = virStorageVolDefParseFile(pool->def, xmlfile); + privvol = virStorageVolDefParseFile(pool->def, xmlfile, 0); else - privvol = virStorageVolDefParseString(pool->def, xmldesc); + privvol = virStorageVolDefParseString(pool->def, xmldesc, 0); if (privvol == NULL) goto cleanup; @@ -1335,7 +1335,7 @@ parallelsStorageVolCreateXMLFrom(virStoragePoolPtr pool, goto cleanup; } - privvol = virStorageVolDefParseString(privpool->def, xmldesc); + privvol = virStorageVolDefParseString(privpool->def, xmldesc, 0); if (privvol == NULL) goto cleanup; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index d05f897..dbf76e2 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2026,7 +2026,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, goto err; } - if ((voldef = virStorageVolDefParseString(spdef, xml)) == NULL) { + if ((voldef = virStorageVolDefParseString(spdef, xml, 0)) == NULL) { VIR_ERROR(_("Error parsing volume XML.")); goto err; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ad92c9b..bc16e87 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1649,7 +1649,7 @@ storageVolCreateXML(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup; - voldef = virStorageVolDefParseString(pool->def, xmldesc); + voldef = virStorageVolDefParseString(pool->def, xmldesc, 0); if (voldef == NULL) goto cleanup; @@ -1810,7 +1810,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } - newvol = virStorageVolDefParseString(pool->def, xmldesc); + newvol = virStorageVolDefParseString(pool->def, xmldesc, 0); if (newvol == NULL) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a386270..718e84a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1230,7 +1230,7 @@ testOpenVolumesForPool(const char *file, if (!node) goto error; - def = virStorageVolDefParseNode(pool->def, ctxt->doc, node); + def = virStorageVolDefParseNode(pool->def, ctxt->doc, node, 0); if (!def) goto error; @@ -5430,7 +5430,7 @@ testStorageVolCreateXML(virStoragePoolPtr pool, goto cleanup; } - privvol = virStorageVolDefParseString(privpool->def, xmldesc); + privvol = virStorageVolDefParseString(privpool->def, xmldesc, 0); if (privvol == NULL) goto cleanup; @@ -5504,7 +5504,7 @@ testStorageVolCreateXMLFrom(virStoragePoolPtr pool, goto cleanup; } - privvol = virStorageVolDefParseString(privpool->def, xmldesc); + privvol = virStorageVolDefParseString(privpool->def, xmldesc, 0); if (privvol == NULL) goto cleanup; diff --git a/src/vbox/vbox_storage.c b/src/vbox/vbox_storage.c index 599f917..4367009 100644 --- a/src/vbox/vbox_storage.c +++ b/src/vbox/vbox_storage.c @@ -430,7 +430,7 @@ vboxStorageVolCreateXML(virStoragePoolPtr pool, memset(&poolDef, 0, sizeof(poolDef)); poolDef.type = VIR_STORAGE_POOL_DIR; - if ((def = virStorageVolDefParseString(&poolDef, xml)) == NULL) + if ((def = virStorageVolDefParseString(&poolDef, xml, 0)) == NULL) goto cleanup; if (!def->name || diff --git a/tests/storagebackendsheepdogtest.c b/tests/storagebackendsheepdogtest.c index 14fc76d..c85a9b4 100644 --- a/tests/storagebackendsheepdogtest.c +++ b/tests/storagebackendsheepdogtest.c @@ -100,7 +100,7 @@ test_vdi_list_parser(collie_test test, char *poolxml, char *volxml) if (!(pool = virStoragePoolDefParseString(poolXmlData))) goto cleanup; - if (!(vol = virStorageVolDefParseString(pool, volXmlData))) + if (!(vol = virStorageVolDefParseString(pool, volXmlData, 0))) goto cleanup; if (VIR_STRDUP(output, test.output) < 0) diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index f9d2d2d..c1f8a9d 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -84,11 +84,11 @@ testCompareXMLToArgvFiles(bool shouldFail, goto cleanup; } - if (!(vol = virStorageVolDefParseString(pool, volXmlData))) + if (!(vol = virStorageVolDefParseString(pool, volXmlData, 0))) goto cleanup; if (inputvolxml && - !(inputvol = virStorageVolDefParseString(inputpool, inputvolXmlData))) + !(inputvol = virStorageVolDefParseString(inputpool, inputvolXmlData, 0))) goto cleanup; testSetVolumeType(vol, pool); diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index cf4d401..ee0495b 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -18,7 +18,7 @@ static int testCompareXMLToXMLFiles(const char *poolxml, const char *inxml, - const char *outxml) + const char *outxml, unsigned long flags) { char *poolXmlData = NULL; char *inXmlData = NULL; @@ -38,7 +38,7 @@ testCompareXMLToXMLFiles(const char *poolxml, const char *inxml, if (!(pool = virStoragePoolDefParseString(poolXmlData))) goto fail; - if (!(dev = virStorageVolDefParseString(pool, inXmlData))) + if (!(dev = virStorageVolDefParseString(pool, inXmlData, flags))) goto fail; if (!(actual = virStorageVolDefFormat(pool, dev))) @@ -64,6 +64,7 @@ testCompareXMLToXMLFiles(const char *poolxml, const char *inxml, struct testInfo { const char *pool; const char *name; + unsigned long flags; }; static int @@ -84,7 +85,7 @@ testCompareXMLToXMLHelper(const void *data) goto cleanup; } - result = testCompareXMLToXMLFiles(poolxml, inxml, outxml); + result = testCompareXMLToXMLFiles(poolxml, inxml, outxml, info->flags); cleanup: VIR_FREE(poolxml); @@ -100,15 +101,17 @@ mymain(void) { int ret = 0; -#define DO_TEST(pool, name) \ +#define DO_TEST_FULL(pool, name, flags) \ do { \ - struct testInfo info = { pool, name }; \ + struct testInfo info = { pool, name, flags }; \ if (virtTestRun("Storage Vol XML-2-XML " name, \ testCompareXMLToXMLHelper, &info) < 0) \ ret = -1; \ } \ while (0); +#define DO_TEST(pool, name) DO_TEST_FULL(pool, name, 0) + DO_TEST("pool-dir", "vol-file"); DO_TEST("pool-dir", "vol-file-naming"); DO_TEST("pool-dir", "vol-file-backing"); -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:09 +0100, Ján Tomko wrote:
Allow the callers to pass down libvirt-internal flags. --- src/conf/storage_conf.c | 25 ++++++++++++++++--------- src/conf/storage_conf.h | 9 ++++++--- src/esx/esx_storage_backend_vmfs.c | 4 ++-- src/parallels/parallels_storage.c | 6 +++--- src/phyp/phyp_driver.c | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 6 +++--- src/vbox/vbox_storage.c | 2 +- tests/storagebackendsheepdogtest.c | 2 +- tests/storagevolxml2argvtest.c | 4 ++-- tests/storagevolxml2xmltest.c | 13 ++++++++----- 11 files changed, 45 insertions(+), 32 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e9aaa8a..c8a860b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1245,7 +1245,8 @@ virStorageSize(const char *unit,
static virStorageVolDefPtr virStorageVolDefParseXML(virStoragePoolDefPtr pool, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned long flags)
Almost every other place in libvirt uses an integer for flags. Why did you use a long?
{ virStorageVolDefPtr ret; virStorageVolOptionsPtr options; @@ -1259,6 +1260,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, size_t i; int n;
+ virCheckFlags(0, NULL); + options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) return NULL;
... ACK after the release if you explain why you used the "long" type or change it to int. Peter

On Wed, Feb 25, 2015 at 09:17:26AM +0100, Peter Krempa wrote:
On Thu, Feb 19, 2015 at 15:59:09 +0100, Ján Tomko wrote:
Allow the callers to pass down libvirt-internal flags. --- src/conf/storage_conf.c | 25 ++++++++++++++++--------- src/conf/storage_conf.h | 9 ++++++--- src/esx/esx_storage_backend_vmfs.c | 4 ++-- src/parallels/parallels_storage.c | 6 +++--- src/phyp/phyp_driver.c | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 6 +++--- src/vbox/vbox_storage.c | 2 +- tests/storagebackendsheepdogtest.c | 2 +- tests/storagevolxml2argvtest.c | 4 ++-- tests/storagevolxml2xmltest.c | 13 ++++++++----- 11 files changed, 45 insertions(+), 32 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e9aaa8a..c8a860b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1245,7 +1245,8 @@ virStorageSize(const char *unit,
static virStorageVolDefPtr virStorageVolDefParseXML(virStoragePoolDefPtr pool, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned long flags)
Almost every other place in libvirt uses an integer for flags. Why did you use a long?
I have absolutely no excuse for the blatant waste of memory and screen space.
{ virStorageVolDefPtr ret; virStorageVolOptionsPtr options; @@ -1259,6 +1260,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, size_t i; int n;
+ virCheckFlags(0, NULL); + options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) return NULL;
...
ACK after the release if you explain why you used the "long" type or change it to int.
I have changed the type to unsigned int in my local branch. Jan

So we can allow omitting the capacity element if backing store is present. --- src/conf/storage_conf.c | 62 ++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index c8a860b..00cea64 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1289,6 +1289,37 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, } } + if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { + if (VIR_ALLOC(ret->target.backingStore) < 0) + goto error; + + ret->target.backingStore->path = backingStore; + backingStore = NULL; + + if (options->formatFromString) { + char *format = virXPathString("string(./backingStore/format/@type)", ctxt); + if (format == NULL) + ret->target.backingStore->format = options->defaultFormat; + else + ret->target.backingStore->format = (options->formatFromString)(format); + + if (ret->target.backingStore->format < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown volume format type %s"), format); + VIR_FREE(format); + goto error; + } + VIR_FREE(format); + } + + if (VIR_ALLOC(ret->target.backingStore->perms) < 0) + goto error; + if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, + "./backingStore/permissions", + DEFAULT_VOL_PERM_MODE) < 0) + goto error; + } + capacity = virXPathString("string(./capacity)", ctxt); unit = virXPathString("string(./capacity/@unit)", ctxt); if (capacity == NULL) { @@ -1341,37 +1372,6 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, goto error; } - if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { - if (VIR_ALLOC(ret->target.backingStore) < 0) - goto error; - - ret->target.backingStore->path = backingStore; - backingStore = NULL; - - if (options->formatFromString) { - char *format = virXPathString("string(./backingStore/format/@type)", ctxt); - if (format == NULL) - ret->target.backingStore->format = options->defaultFormat; - else - ret->target.backingStore->format = (options->formatFromString)(format); - - if (ret->target.backingStore->format < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown volume format type %s"), format); - VIR_FREE(format); - goto error; - } - VIR_FREE(format); - } - - if (VIR_ALLOC(ret->target.backingStore->perms) < 0) - goto error; - if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, - "./backingStore/permissions", - DEFAULT_VOL_PERM_MODE) < 0) - goto error; - } - ret->target.compat = virXPathString("string(./target/compat)", ctxt); if (ret->target.compat) { char **version = virStringSplit(ret->target.compat, ".", 2); -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:10 +0100, Ján Tomko wrote:
So we can allow omitting the capacity element if backing store is present. --- src/conf/storage_conf.c | 62 ++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
ACK after the release. Peter

Add VIR_VOL_XML_PARSE_NO_CAPACITY flag to the volume XML parser. When set, it allows the capacity element to be omitted. --- src/conf/storage_conf.c | 12 ++++++------ src/conf/storage_conf.h | 4 ++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 00cea64..ca8fc9b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1260,7 +1260,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, size_t i; int n; - virCheckFlags(0, NULL); + virCheckFlags(VIR_VOL_XML_PARSE_NO_CAPACITY, NULL); options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) @@ -1322,13 +1322,13 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, capacity = virXPathString("string(./capacity)", ctxt); unit = virXPathString("string(./capacity/@unit)", ctxt); - if (capacity == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing capacity element")); + if (capacity) { + if (virStorageSize(unit, capacity, &ret->target.capacity) < 0) + goto error; + } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY)) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element")); goto error; } - if (virStorageSize(unit, capacity, &ret->target.capacity) < 0) - goto error; VIR_FREE(unit); allocation = virXPathString("string(./allocation)", ctxt); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 2162426..cd3d09c 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -345,6 +345,10 @@ virStoragePoolDefPtr virStoragePoolDefParseNode(xmlDocPtr xml, xmlNodePtr root); char *virStoragePoolDefFormat(virStoragePoolDefPtr def); +typedef enum { + /* do not require volume capacity at all */ + VIR_VOL_XML_PARSE_NO_CAPACITY = 1 << 0, +} virStorageVolDefParseFlags; virStorageVolDefPtr virStorageVolDefParseString(virStoragePoolDefPtr pool, const char *xml, -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:11 +0100, Ján Tomko wrote:
Add VIR_VOL_XML_PARSE_NO_CAPACITY flag to the volume XML parser. When set, it allows the capacity element to be omitted. --- src/conf/storage_conf.c | 12 ++++++------ src/conf/storage_conf.h | 4 ++++ 2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 00cea64..ca8fc9b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c
ACK after the release. Peter

We can get the capacity from the input volume. --- src/storage/storage_backend.c | 2 +- src/storage/storage_driver.c | 3 ++- .../qcow2-nocapacity-convert-prealloc.argv | 4 ++++ tests/storagevolxml2argvtest.c | 9 ++++++++- tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml | 19 +++++++++++++++++++ tests/storagevolxml2xmlout/vol-qcow2-nocapacity.xml | 21 +++++++++++++++++++++ tests/storagevolxml2xmltest.c | 2 ++ 7 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-nocapacity.xml diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a67d50c..dd33436 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1055,7 +1055,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (convert) virCommandAddArg(cmd, inputPath); virCommandAddArg(cmd, vol->target.path); - if (!convert) + if (!convert && size_arg) virCommandAddArgFormat(cmd, "%lluK", size_arg); return cmd; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index bc16e87..409b486 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1810,7 +1810,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } - newvol = virStorageVolDefParseString(pool->def, xmldesc, 0); + newvol = virStorageVolDefParseString(pool->def, xmldesc, + VIR_VOL_XML_PARSE_NO_CAPACITY); if (newvol == NULL) goto cleanup; diff --git a/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv b/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv new file mode 100644 index 0000000..9073b1b --- /dev/null +++ b/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv @@ -0,0 +1,4 @@ +qemu-img convert -f raw -O qcow2 \ +-o encryption=on,preallocation=metadata \ +/var/lib/libvirt/images/sparse.img \ +/var/lib/libvirt/images/OtherDemo.img diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index c1f8a9d..52bb856 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -49,6 +49,7 @@ testCompareXMLToArgvFiles(bool shouldFail, char *expectedCmdline = NULL; char *actualCmdline = NULL; int ret = -1; + unsigned long parse_flags = 0; int len; @@ -84,7 +85,10 @@ testCompareXMLToArgvFiles(bool shouldFail, goto cleanup; } - if (!(vol = virStorageVolDefParseString(pool, volXmlData, 0))) + if (inputvolxml) + parse_flags |= VIR_VOL_XML_PARSE_NO_CAPACITY; + + if (!(vol = virStorageVolDefParseString(pool, volXmlData, parse_flags))) goto cleanup; if (inputvolxml && @@ -305,6 +309,9 @@ mymain(void) DO_TEST("pool-dir", "vol-qcow2-nocow", NULL, NULL, "qcow2-nocow-compat", 0, FMT_COMPAT); + DO_TEST("pool-dir", "vol-qcow2-nocapacity", + "pool-dir", "vol-file", + "qcow2-nocapacity-convert-prealloc", flags, FMT_OPTIONS); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml b/tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml new file mode 100644 index 0000000..9746900 --- /dev/null +++ b/tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml @@ -0,0 +1,19 @@ +<volume> + <name>OtherDemo.img</name> + <key>/var/lib/libvirt/images/OtherDemo.img</key> + <source> + </source> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + </target> +</volume> diff --git a/tests/storagevolxml2xmlout/vol-qcow2-nocapacity.xml b/tests/storagevolxml2xmlout/vol-qcow2-nocapacity.xml new file mode 100644 index 0000000..223e689 --- /dev/null +++ b/tests/storagevolxml2xmlout/vol-qcow2-nocapacity.xml @@ -0,0 +1,21 @@ +<volume type='file'> + <name>OtherDemo.img</name> + <key>/var/lib/libvirt/images/OtherDemo.img</key> + <source> + </source> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + </target> +</volume> diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index ee0495b..e04b4ef 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -126,6 +126,8 @@ mymain(void) DO_TEST("pool-sheepdog", "vol-sheepdog"); DO_TEST("pool-gluster", "vol-gluster-dir"); DO_TEST("pool-gluster", "vol-gluster-dir-neg-uid"); + DO_TEST_FULL("pool-dir", "vol-qcow2-nocapacity", + VIR_VOL_XML_PARSE_NO_CAPACITY); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:12 +0100, Ján Tomko wrote:
We can get the capacity from the input volume.
I had to trace the code to understand what this patch was about ...
--- src/storage/storage_backend.c | 2 +- src/storage/storage_driver.c | 3 ++- .../qcow2-nocapacity-convert-prealloc.argv | 4 ++++ tests/storagevolxml2argvtest.c | 9 ++++++++- tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml | 19 +++++++++++++++++++ tests/storagevolxml2xmlout/vol-qcow2-nocapacity.xml | 21 +++++++++++++++++++++ tests/storagevolxml2xmltest.c | 2 ++ 7 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-nocapacity.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-nocapacity.xml
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a67d50c..dd33436 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1055,7 +1055,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (convert) virCommandAddArg(cmd, inputPath); virCommandAddArg(cmd, vol->target.path); - if (!convert) + if (!convert && size_arg)
Note that this change is on the "!convert" (create) path ... [1]
virCommandAddArgFormat(cmd, "%lluK", size_arg);
return cmd; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index bc16e87..409b486 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1810,7 +1810,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; }
- newvol = virStorageVolDefParseString(pool->def, xmldesc, 0); + newvol = virStorageVolDefParseString(pool->def, xmldesc, + VIR_VOL_XML_PARSE_NO_CAPACITY); if (newvol == NULL) goto cleanup;
Few lines below this change there's the following hunk: /* Is there ever a valid case for this? */ if (newvol->target.capacity < origvol->target.capacity) newvol->target.capacity = origvol->target.capacity; Which sets the capacity to the capacity of the original volume. While this is probably semantically OK it might be worth tweaking the comment.
diff --git a/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv b/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv new file mode 100644 index 0000000..9073b1b --- /dev/null +++ b/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv @@ -0,0 +1,4 @@ +qemu-img convert -f raw -O qcow2 \
[1] ... while the test you are adding is testing the convert path. I think that the hunk [1] belongs to a different patch as the size argument was added only on the "create" path anyways.
+-o encryption=on,preallocation=metadata \ +/var/lib/libvirt/images/sparse.img \ +/var/lib/libvirt/images/OtherDemo.img
ACK after the release with hunk [1] removed. Peter

On Wed, Feb 25, 2015 at 10:05:35AM +0100, Peter Krempa wrote:
On Thu, Feb 19, 2015 at 15:59:12 +0100, Ján Tomko wrote:
We can get the capacity from the input volume.
I had to trace the code to understand what this patch was about ...
I have added a commit message: In virStorageVolCreateXML, add VIR_VOL_XML_PARSE_NO_CAPACITY to the call parsing the XML of the new volume to make the capacity optional. If the capacity is omitted, use the capacity of the old volume. We already do that for values that are less than the original volume capacity.
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a67d50c..dd33436 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1055,7 +1055,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (convert) virCommandAddArg(cmd, inputPath); virCommandAddArg(cmd, vol->target.path); - if (!convert) + if (!convert && size_arg)
Note that this change is on the "!convert" (create) path ... [1]
virCommandAddArgFormat(cmd, "%lluK", size_arg);
return cmd; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index bc16e87..409b486 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1810,7 +1810,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; }
- newvol = virStorageVolDefParseString(pool->def, xmldesc, 0); + newvol = virStorageVolDefParseString(pool->def, xmldesc, + VIR_VOL_XML_PARSE_NO_CAPACITY); if (newvol == NULL) goto cleanup;
Few lines below this change there's the following hunk:
/* Is there ever a valid case for this? */ if (newvol->target.capacity < origvol->target.capacity) newvol->target.capacity = origvol->target.capacity;
Which sets the capacity to the capacity of the original volume. While this is probably semantically OK it might be worth tweaking the comment.
tweaked the comment: /* Use the original volume's capacity in case the new capacity * is less than that, or it was omitted */
diff --git a/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv b/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv new file mode 100644 index 0000000..9073b1b --- /dev/null +++ b/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv @@ -0,0 +1,4 @@ +qemu-img convert -f raw -O qcow2 \
[1] ... while the test you are adding is testing the convert path. I think that the hunk [1] belongs to a different patch as the size argument was added only on the "create" path anyways.
+-o encryption=on,preallocation=metadata \ +/var/lib/libvirt/images/sparse.img \ +/var/lib/libvirt/images/OtherDemo.img
ACK after the release with hunk [1] removed.
and removed the unrelated hunk. Jan

Do not use relPath, it has not been filled by virStorageFileMetadataNew. --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8d3d1f5..029fe17 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1030,12 +1030,12 @@ virStorageFileGetMetadataFromFD(const char *path, } if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { - virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->relPath); + virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->path); goto cleanup; } if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), meta->relPath); + virReportSystemError(errno, _("cannot read header '%s'"), meta->path); goto cleanup; } -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:13 +0100, Ján Tomko wrote:
Do not use relPath, it has not been filled by virStorageFileMetadataNew. --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8d3d1f5..029fe17 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1030,12 +1030,12 @@ virStorageFileGetMetadataFromFD(const char *path, }
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { - virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->relPath); + virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->path); goto cleanup; }
if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), meta->relPath); + virReportSystemError(errno, _("cannot read header '%s'"), meta->path); goto cleanup; }
ACK, this is an artifact of the changes done when refactoring to use the virStorageSource structure everywhere. Safe for release. Peter

On Wed, Feb 25, 2015 at 09:49:31AM +0100, Peter Krempa wrote:
On Thu, Feb 19, 2015 at 15:59:13 +0100, Ján Tomko wrote:
Do not use relPath, it has not been filled by virStorageFileMetadataNew. --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8d3d1f5..029fe17 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1030,12 +1030,12 @@ virStorageFileGetMetadataFromFD(const char *path, }
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { - virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->relPath); + virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->path); goto cleanup; }
if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), meta->relPath); + virReportSystemError(errno, _("cannot read header '%s'"), meta->path); goto cleanup; }
ACK, this is an artifact of the changes done when refactoring to use the virStorageSource structure everywhere.
Safe for release.
I have pushed this one out of the (arbitrary) order.
Peter
Jan

Instead of just looking at the output of fstat, call virStorageFileGetMetadata to get the full capacity from image headers. --- src/storage/storage_backend.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index dd33436..8471744 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1514,6 +1514,9 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, { int ret, fd = -1; struct stat sb; + virStorageSourcePtr meta = NULL; + char *buf = NULL; + ssize_t len = VIR_STORAGE_MAX_HEADER; if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) goto cleanup; @@ -1523,14 +1526,41 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, updateCapacity)) < 0) goto cleanup; + if (target->type == VIR_STORAGE_VOL_FILE && + target->format != VIR_STORAGE_FILE_NONE) { + if (S_ISDIR(sb.st_mode)) { + ret = 0; + goto cleanup; + } + + if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { + virReportSystemError(errno, _("cannot seek to start of '%s'"), target->path); + goto cleanup; + } + + if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { + virReportSystemError(errno, _("cannot read header '%s'"), target->path); + goto cleanup; + } + + if (!(meta = virStorageFileGetMetadataFromBuf(target->path, buf, len, target->format, + NULL))) { + goto cleanup; + } + + if (meta->capacity) + target->capacity = meta->capacity; + } + if (withBlockVolFormat) { if ((ret = virStorageBackendDetectBlockVolFormatFD(target, fd)) < 0) goto cleanup; } cleanup: + virStorageSourceFree(meta); VIR_FORCE_CLOSE(fd); - + VIR_FREE(buf); return ret; } -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:14 +0100, Ján Tomko wrote:
Instead of just looking at the output of fstat, call virStorageFileGetMetadata to get the full capacity from image headers. --- src/storage/storage_backend.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index dd33436..8471744 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1514,6 +1514,9 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, {
This function has a parameter named 'updateCapacity' ...
int ret, fd = -1; struct stat sb; + virStorageSourcePtr meta = NULL; + char *buf = NULL; + ssize_t len = VIR_STORAGE_MAX_HEADER;
if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) goto cleanup; @@ -1523,14 +1526,41 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, updateCapacity)) < 0)
... that is obeyed when updating the capacity via the stat call ...
goto cleanup;
+ if (target->type == VIR_STORAGE_VOL_FILE && + target->format != VIR_STORAGE_FILE_NONE) {
... but this new code ignores it. Is there a specific reason for that?
+ if (S_ISDIR(sb.st_mode)) { + ret = 0; + goto cleanup; + } + + if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { + virReportSystemError(errno, _("cannot seek to start of '%s'"), target->path); + goto cleanup;
Peter

On Wed, Feb 25, 2015 at 10:10:38 +0100, Peter Krempa wrote:
On Thu, Feb 19, 2015 at 15:59:14 +0100, Ján Tomko wrote:
Instead of just looking at the output of fstat, call virStorageFileGetMetadata to get the full capacity from image headers. --- src/storage/storage_backend.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index dd33436..8471744 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1514,6 +1514,9 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, {
This function has a parameter named 'updateCapacity' ...
int ret, fd = -1; struct stat sb; + virStorageSourcePtr meta = NULL; + char *buf = NULL; + ssize_t len = VIR_STORAGE_MAX_HEADER;
if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) goto cleanup; @@ -1523,14 +1526,41 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, updateCapacity)) < 0)
... that is obeyed when updating the capacity via the stat call ...
goto cleanup;
+ if (target->type == VIR_STORAGE_VOL_FILE && + target->format != VIR_STORAGE_FILE_NONE) {
... but this new code ignores it. Is there a specific reason for that?
I see. Next patch reverts all the changes regarding update capacity. ACK after the release to this patch. Peter

This reverts commit f1856eb622fde2e6c3a6a932d1dded7f1691d205. Now that we can update capacity from image metadata, we don't need to skip the update. --- src/storage/storage_backend.c | 20 +++++--------------- src/storage/storage_backend.h | 5 +---- src/storage/storage_backend_disk.c | 4 ++-- src/storage/storage_backend_fs.c | 10 ++++------ src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 16 insertions(+), 31 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 8471744..ea5cabe 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1508,7 +1508,6 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, - bool updateCapacity, bool withBlockVolFormat, unsigned int openflags) { @@ -1522,8 +1521,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, goto cleanup; fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, - updateCapacity)) < 0) + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) goto cleanup; if (target->type == VIR_STORAGE_VOL_FILE && @@ -1566,21 +1564,18 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, - bool updateCapacity, bool withBlockVolFormat, unsigned int openflags) { int ret; if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, - updateCapacity, withBlockVolFormat, openflags)) < 0) return ret; if (vol->target.backingStore && (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, - updateCapacity, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT | VIR_STORAGE_VOL_OPEN_NOERROR) < 0)) @@ -1594,15 +1589,13 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, * @target: target definition ptr of volume to update * @fd: fd of storage volume to update, via virStorageBackendOpenVol*, or -1 * @sb: details about file (must match @fd, if that is provided) - * @updateCapacity: If true, updated capacity info will be stored * * Returns 0 for success, -1 on a legitimate error condition. */ int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb, - bool updateCapacity) + struct stat *sb) { #if WITH_SELINUX security_context_t filecon = NULL; @@ -1618,12 +1611,10 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, /* Regular files may be sparse, so logical size (capacity) is not same * as actual allocation above */ - if (updateCapacity) - target->capacity = sb->st_size; + target->capacity = sb->st_size; } else if (S_ISDIR(sb->st_mode)) { target->allocation = 0; - if (updateCapacity) - target->capacity = 0; + target->capacity = 0; } else if (fd >= 0) { off_t end; /* XXX this is POSIX compliant, but doesn't work for CHAR files, @@ -1639,8 +1630,7 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return -1; } target->allocation = end; - if (updateCapacity) - target->capacity = end; + target->capacity = end; } if (!target->perms && VIR_ALLOC(target->perms) < 0) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index a93df5d..9f1db60 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -179,17 +179,14 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, - bool updateCapacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, - bool updateCapacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb, - bool updateCapacity); + struct stat *sb); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 9f4d76a..f62b23e 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -152,14 +152,14 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, * -1 was returned indicating some other error than an open error. */ if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { - if (virStorageBackendUpdateVolInfo(vol, true, false, + if (virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_OPEN_DEFAULT | VIR_STORAGE_VOL_OPEN_NOERROR) == -1) return -1; vol->target.allocation = vol->target.capacity = (vol->source.extents[0].end - vol->source.extents[0].start); } else { - if (virStorageBackendUpdateVolInfo(vol, true, false, + if (virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) return -1; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 2af5dd7..77d894c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -80,7 +80,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, return rc; /* Take care to propagate rc, it is not always -1 */ fd = rc; - if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, true) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb) < 0) goto cleanup; if (S_ISDIR(sb.st_mode)) { @@ -902,7 +902,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.backingStore) { ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, - true, false, + false, VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, * the capacity, allocation, owner, group and mode are unknown. @@ -1183,10 +1183,8 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, { int ret; - /* Refresh allocation / permissions info in case its changed - * don't update the capacity value for this pass - */ - ret = virStorageBackendUpdateVolInfo(vol, false, false, + /* Refresh allocation / capacity / permissions info in case its changed */ + ret = virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_FS_OPEN_FLAGS); if (ret < 0) return ret; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 53e4632..d2e79bc 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -268,7 +268,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (VIR_ALLOC(vol) < 0) goto cleanup; - if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st, true) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st) < 0) goto cleanup; if (virStorageBackendGlusterSetMetadata(state, vol, name) < 0) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 8aa68a6..7ba8ded 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -161,7 +161,7 @@ virStorageBackendLogicalMakeVol(char **const groups, if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, true, false, + if (virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) goto cleanup; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index b67dc2a..44bcd60 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -60,7 +60,7 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, true, true, + if (virStorageBackendUpdateVolInfo(vol, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { goto cleanup; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c4879b0..58e7e6d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -206,7 +206,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; } - if (virStorageBackendUpdateVolInfo(vol, true, true, + if (virStorageBackendUpdateVolInfo(vol, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { retval = -1; goto free_vol; -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:15 +0100, Ján Tomko wrote:
This reverts commit f1856eb622fde2e6c3a6a932d1dded7f1691d205.
Now that we can update capacity from image metadata, we don't need to skip the update. --- src/storage/storage_backend.c | 20 +++++--------------- src/storage/storage_backend.h | 5 +---- src/storage/storage_backend_disk.c | 4 ++-- src/storage/storage_backend_fs.c | 10 ++++------ src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 16 insertions(+), 31 deletions(-)
ACK, the code is no longer necessary once we update the capacity properly. Peter

Add VIR_VOL_XML_PARSE_OPT_CAPACITY flag to virStorageVolDefParseXML. With this flag, no error is reported when the capacity is missing if there is a backing store. --- src/conf/storage_conf.c | 6 ++++-- src/conf/storage_conf.h | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ca8fc9b..9723696 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1260,7 +1260,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, size_t i; int n; - virCheckFlags(VIR_VOL_XML_PARSE_NO_CAPACITY, NULL); + virCheckFlags(VIR_VOL_XML_PARSE_NO_CAPACITY | + VIR_VOL_XML_PARSE_OPT_CAPACITY, NULL); options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) @@ -1325,7 +1326,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (capacity) { if (virStorageSize(unit, capacity, &ret->target.capacity) < 0) goto error; - } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY)) { + } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) && + !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && ret->target.backingStore)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element")); goto error; } diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index cd3d09c..21cd584 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -348,6 +348,8 @@ char *virStoragePoolDefFormat(virStoragePoolDefPtr def); typedef enum { /* do not require volume capacity at all */ VIR_VOL_XML_PARSE_NO_CAPACITY = 1 << 0, + /* do not require volume capacity if the volume has a backing store */ + VIR_VOL_XML_PARSE_OPT_CAPACITY = 1 << 1, } virStorageVolDefParseFlags; virStorageVolDefPtr virStorageVolDefParseString(virStoragePoolDefPtr pool, -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:16 +0100, Ján Tomko wrote:
Add VIR_VOL_XML_PARSE_OPT_CAPACITY flag to virStorageVolDefParseXML. With this flag, no error is reported when the capacity is missing if there is a backing store. --- src/conf/storage_conf.c | 6 ++++-- src/conf/storage_conf.h | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-)
ACK after the release. Peter

The tool creating the image can get the capacity from the backing storage. Just refresh the volume afterwards. https://bugzilla.redhat.com/show_bug.cgi?id=958510 --- src/storage/storage_backend.c | 6 ++++++ src/storage/storage_backend_fs.c | 7 +++++++ src/storage/storage_backend_rbd.c | 6 ++++++ src/storage/storage_backend_sheepdog.c | 6 ++++++ src/storage/storage_driver.c | 14 ++++++++++++- tests/storagevolxml2argvdata/qcow2-nocapacity.argv | 5 +++++ tests/storagevolxml2argvtest.c | 20 +++++++++++-------- .../vol-qcow2-nocapacity-backing.xml | 23 ++++++++++++++++++++++ 8 files changed, 78 insertions(+), 9 deletions(-) create mode 100644 tests/storagevolxml2argvdata/qcow2-nocapacity.argv create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-nocapacity-backing.xml diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index ea5cabe..a4e75f5a5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -487,6 +487,12 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } + if (vol->target.backingStore) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("backing storage not supported for raw volumes")); + goto cleanup; + } + if (flags & VIR_STORAGE_VOL_CREATE_REFLINK) reflink_copy = true; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 77d894c..35385db 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1037,6 +1037,13 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } + if (vol->target.backingStore) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("backing storage not supported for directories volumes")); + return -1; + } + + if ((err = virDirCreate(vol->target.path, vol->target.perms->mode, vol->target.perms->uid, vol->target.perms->gid, diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 57182de..ae4bcb3 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -509,6 +509,12 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, virCheckFlags(0, -1); + if (!vol->target.capacity) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("volume capacity required for this storage pool")); + goto cleanup; + } + if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) goto cleanup; diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 9419859..f389d9b 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -266,6 +266,12 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn, virCheckFlags(0, -1); + if (!vol->target.capacity) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("volume capacity required for this pool")); + goto cleanup; + } + virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "create", vol->name, NULL); virCommandAddArgFormat(cmd, "%llu", vol->target.capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 409b486..e8af90f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1649,10 +1649,18 @@ storageVolCreateXML(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup; - voldef = virStorageVolDefParseString(pool->def, xmldesc, 0); + voldef = virStorageVolDefParseString(pool->def, xmldesc, + VIR_VOL_XML_PARSE_OPT_CAPACITY); if (voldef == NULL) goto cleanup; + if (!voldef->target.capacity && !backend->buildVol) { + virReportError(VIR_ERR_NO_SUPPORT, + "%s", _("volume capacity required for this " + "storage pool")); + goto cleanup; + } + if (virStorageVolCreateXMLEnsureACL(obj->conn, pool->def, voldef) < 0) goto cleanup; @@ -1725,6 +1733,10 @@ storageVolCreateXML(virStoragePoolPtr obj, } + if (backend->refreshVol && + backend->refreshVol(obj->conn, pool, voldef) < 0) + goto cleanup; + /* Update pool metadata */ pool->def->allocation += buildvoldef->target.allocation; pool->def->available -= buildvoldef->target.allocation; diff --git a/tests/storagevolxml2argvdata/qcow2-nocapacity.argv b/tests/storagevolxml2argvdata/qcow2-nocapacity.argv new file mode 100644 index 0000000..1198cba --- /dev/null +++ b/tests/storagevolxml2argvdata/qcow2-nocapacity.argv @@ -0,0 +1,5 @@ +qemu-img create \ +-f qcow2 \ +-b /dev/null \ +-o backing_fmt=raw,encryption=on \ +/var/lib/libvirt/images/OtherDemo.img diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 52bb856..696659c 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -40,7 +40,8 @@ testCompareXMLToArgvFiles(bool shouldFail, const char *inputvolxml, const char *cmdline, unsigned int flags, - int imgformat) + int imgformat, + unsigned long parse_flags) { char *volXmlData = NULL; char *poolXmlData = NULL; @@ -49,7 +50,6 @@ testCompareXMLToArgvFiles(bool shouldFail, char *expectedCmdline = NULL; char *actualCmdline = NULL; int ret = -1; - unsigned long parse_flags = 0; int len; @@ -149,6 +149,7 @@ struct testInfo { const char *cmdline; unsigned int flags; int imgformat; + unsigned long parseflags; }; static int @@ -183,7 +184,7 @@ testCompareXMLToArgvHelper(const void *data) result = testCompareXMLToArgvFiles(info->shouldFail, poolxml, volxml, inputpoolxml, inputvolxml, cmdline, info->flags, - info->imgformat); + info->imgformat, info->parseflags); cleanup: VIR_FREE(poolxml); @@ -210,11 +211,11 @@ mymain(void) int ret = 0; unsigned int flags = VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA; -#define DO_TEST_FULL(shouldFail, pool, vol, inputpool, inputvol, cmdline, \ - flags, imgformat) \ +#define DO_TEST_FULL(shouldFail, parseflags, pool, vol, inputpool, inputvol, \ + cmdline, flags, imgformat) \ do { \ struct testInfo info = { shouldFail, pool, vol, inputpool, inputvol, \ - cmdline, flags, imgformat }; \ + cmdline, flags, imgformat, parseflags }; \ if (virtTestRun("Storage Vol XML-2-argv " cmdline, \ testCompareXMLToArgvHelper, &info) < 0) \ ret = -1; \ @@ -222,10 +223,10 @@ mymain(void) while (0); #define DO_TEST(pool, ...) \ - DO_TEST_FULL(false, pool, __VA_ARGS__) + DO_TEST_FULL(false, 0, pool, __VA_ARGS__) #define DO_TEST_FAIL(pool, ...) \ - DO_TEST_FULL(true, pool, __VA_ARGS__) + DO_TEST_FULL(true, 0, pool, __VA_ARGS__) DO_TEST("pool-dir", "vol-qcow2", NULL, NULL, @@ -312,6 +313,9 @@ mymain(void) DO_TEST("pool-dir", "vol-qcow2-nocapacity", "pool-dir", "vol-file", "qcow2-nocapacity-convert-prealloc", flags, FMT_OPTIONS); + DO_TEST_FULL(false, VIR_VOL_XML_PARSE_OPT_CAPACITY, + "pool-dir", "vol-qcow2-nocapacity-backing", NULL, NULL, + "qcow2-nocapacity", 0, FMT_OPTIONS); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/storagevolxml2xmlin/vol-qcow2-nocapacity-backing.xml b/tests/storagevolxml2xmlin/vol-qcow2-nocapacity-backing.xml new file mode 100644 index 0000000..f8e439b --- /dev/null +++ b/tests/storagevolxml2xmlin/vol-qcow2-nocapacity-backing.xml @@ -0,0 +1,23 @@ +<volume> + <name>OtherDemo.img</name> + <key>/var/lib/libvirt/images/OtherDemo.img</key> + <source> + </source> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + </target> + <backingStore> + <path>/dev/null</path> + <format type='raw'/> + </backingStore> +</volume> -- 2.0.5

On Thu, Feb 19, 2015 at 15:59:17 +0100, Ján Tomko wrote:
The tool creating the image can get the capacity from the backing storage. Just refresh the volume afterwards.
https://bugzilla.redhat.com/show_bug.cgi?id=958510 --- src/storage/storage_backend.c | 6 ++++++ src/storage/storage_backend_fs.c | 7 +++++++ src/storage/storage_backend_rbd.c | 6 ++++++ src/storage/storage_backend_sheepdog.c | 6 ++++++ src/storage/storage_driver.c | 14 ++++++++++++- tests/storagevolxml2argvdata/qcow2-nocapacity.argv | 5 +++++ tests/storagevolxml2argvtest.c | 20 +++++++++++-------- .../vol-qcow2-nocapacity-backing.xml | 23 ++++++++++++++++++++++ 8 files changed, 78 insertions(+), 9 deletions(-) create mode 100644 tests/storagevolxml2argvdata/qcow2-nocapacity.argv create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-nocapacity-backing.xml
This is the correct place for the hunk I was complaining about. ACK after the release. Peter
participants (2)
-
Ján Tomko
-
Peter Krempa