[libvirt] [PATCH 0/3] storage pool capabilities fixes
Pavel Hrdina (3): tests: use virTestCompareToFile in storagepoolcapstest tests: invert the return logic in storagepoolcapstest caps: drop requiredSourceElements from storage pool capabilities docs/formatstoragecaps.html.in | 7 -- src/conf/storage_conf.c | 25 +------ .../storagepoolcapsschemadata/poolcaps-fs.xml | 67 ------------------- .../poolcaps-full.xml | 67 ------------------- tests/storagepoolcapstest.c | 17 ++--- 5 files changed, 7 insertions(+), 176 deletions(-) -- 2.20.1
This will allow to use VIR_TEST_REGENERATE_OUTPUT. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/storagepoolcapstest.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/storagepoolcapstest.c b/tests/storagepoolcapstest.c index d31f50c957..ade586bcdd 100644 --- a/tests/storagepoolcapstest.c +++ b/tests/storagepoolcapstest.c @@ -68,16 +68,11 @@ test_virStoragePoolCapsFormat(const void *opaque) abs_srcdir, data->filename) < 0) goto cleanup; - if (virFileReadAll(path, 8192, &poolCapsFromFile) < 0) - goto cleanup; - if (!(poolCapsXML = virStoragePoolCapsFormat(poolCaps))) goto cleanup; - if (STRNEQ(poolCapsFromFile, poolCapsXML)) { - virTestDifference(stderr, poolCapsFromFile, poolCapsXML); + if (virTestCompareToFile(poolCapsXML, path) < 0) goto cleanup; - } ret = 0; -- 2.20.1
On Wed, Mar 06, 2019 at 19:16:46 +0100, Pavel Hrdina wrote:
This will allow to use VIR_TEST_REGENERATE_OUTPUT.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/storagepoolcapstest.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/tests/storagepoolcapstest.c b/tests/storagepoolcapstest.c index d31f50c957..ade586bcdd 100644 --- a/tests/storagepoolcapstest.c +++ b/tests/storagepoolcapstest.c @@ -68,16 +68,11 @@ test_virStoragePoolCapsFormat(const void *opaque) abs_srcdir, data->filename) < 0) goto cleanup;
- if (virFileReadAll(path, 8192, &poolCapsFromFile) < 0) - goto cleanup; - if (!(poolCapsXML = virStoragePoolCapsFormat(poolCaps))) goto cleanup;
- if (STRNEQ(poolCapsFromFile, poolCapsXML)) { - virTestDifference(stderr, poolCapsFromFile, poolCapsXML); + if (virTestCompareToFile(poolCapsXML, path) < 0) goto cleanup;
/home/pipo/libvirt/tests/storagepoolcapstest.c:60:26: error: unused variable 'poolCapsFromFile' [-Werror,-Wunused-variable] VIR_AUTOFREE(char *) poolCapsFromFile = NULL; ^ ACK with the obvious fix. Also you should consider using clang as a final test before posting.
This way if the first test "full" fails we will run the second test as well. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/storagepoolcapstest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/storagepoolcapstest.c b/tests/storagepoolcapstest.c index ade586bcdd..da21bca546 100644 --- a/tests/storagepoolcapstest.c +++ b/tests/storagepoolcapstest.c @@ -85,7 +85,7 @@ test_virStoragePoolCapsFormat(const void *opaque) static int mymain(void) { - int ret = -1; + int ret = 0; virCapsPtr fullCaps = NULL; virCapsPtr fsCaps = NULL; @@ -94,12 +94,14 @@ mymain(void) struct test_virStoragePoolCapsFormatData data = \ {.filename = Filename, .driverCaps = DriverCaps }; \ if (virTestRun(Filename, test_virStoragePoolCapsFormat, &data) < 0) \ - goto cleanup; \ + ret = -1; \ } while (0) if (!(fullCaps = virCapabilitiesNew(VIR_ARCH_NONE, false, false)) || - !(fsCaps = virCapabilitiesNew(VIR_ARCH_NONE, false, false))) + !(fsCaps = virCapabilitiesNew(VIR_ARCH_NONE, false, false))) { + ret = -1; goto cleanup; + } test_virCapabilitiesAddFullStoragePool(fullCaps); test_virCapabilitiesAddFSStoragePool(fsCaps); @@ -107,8 +109,6 @@ mymain(void) DO_TEST("full", fullCaps); DO_TEST("fs", fsCaps); - ret = 0; - cleanup: virObjectUnref(fullCaps); virObjectUnref(fsCaps); -- 2.20.1
On Wed, Mar 06, 2019 at 19:16:47 +0100, Pavel Hrdina wrote:
This way if the first test "full" fails we will run the second test as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/storagepoolcapstest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
ACK, that's the way almost all our test files use. Alternatively by using VIR_AUTOUNREF for both 'fullCaps' and 'fsCaps' you can get rid of the whole cleanup label and just return -1 if any of the virCapabilitiesNew calls fails. Then use 'ret' only in DO_TEST.
On Thu, Mar 07, 2019 at 09:11:45AM +0100, Peter Krempa wrote:
On Wed, Mar 06, 2019 at 19:16:47 +0100, Pavel Hrdina wrote:
This way if the first test "full" fails we will run the second test as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/storagepoolcapstest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
ACK, that's the way almost all our test files use.
Alternatively by using VIR_AUTOUNREF for both 'fullCaps' and 'fsCaps' you can get rid of the whole cleanup label and just return -1 if any of the virCapabilitiesNew calls fails. Then use 'ret' only in DO_TEST.
Good point, I'll post a followup patch that will use VIR_AUTOUNREF for the whole file. Thanks Pavel
Capabilities should not duplicate data that are obvious from our documentation and will not change with different QEMU binaries or the way how we compile libvirt. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatstoragecaps.html.in | 7 -- src/conf/storage_conf.c | 25 +------ .../storagepoolcapsschemadata/poolcaps-fs.xml | 67 ------------------- .../poolcaps-full.xml | 67 ------------------- 4 files changed, 1 insertion(+), 165 deletions(-) diff --git a/docs/formatstoragecaps.html.in b/docs/formatstoragecaps.html.in index 2813d061b0..ee3888f44d 100644 --- a/docs/formatstoragecaps.html.in +++ b/docs/formatstoragecaps.html.in @@ -56,9 +56,6 @@ <value>ext2</value> ... </enum> - <enum name='requiredSourceElements'> - <value>device</value> - </enum> </poolOptions> <volOptions> <defaultFormat type='raw'</> @@ -92,10 +89,6 @@ <dd>Lists all the possible <code>poolOptions</code> source pool format types. </dd> - <dt><code>requiredSourceElements</code></dt> - <dd>Lists all the required <code>poolOptions</code> source - subelements required for a valid source pool element. - </dd> <dt><code>targetFormatType</code></dt> <dd>Lists all the possible <code>volOptions</code> target volume format types. diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 751a00aaf0..c7ab5b8802 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -366,7 +366,7 @@ virStoragePoolOptionsFormatPool(virBufferPtr buf, if (!(poolOptions = virStoragePoolOptionsForPoolType(type))) return -1; - if (!poolOptions->formatToString && !poolOptions->flags) + if (!poolOptions->formatToString) return 0; virBufferAddLit(buf, "<poolOptions>\n"); @@ -389,29 +389,6 @@ virStoragePoolOptionsFormatPool(virBufferPtr buf, virBufferAddLit(buf, "</enum>\n"); } - if (poolOptions->flags) { - virBufferAddLit(buf, "<enum name='requiredSourceElements'>\n"); - virBufferAdjustIndent(buf, 2); - - if (poolOptions->flags & VIR_STORAGE_POOL_SOURCE_HOST) - virBufferAddLit(buf, "<value>host</value>\n"); - if (poolOptions->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) - virBufferAddLit(buf, "<value>device</value>\n"); - if (poolOptions->flags & VIR_STORAGE_POOL_SOURCE_DIR) - virBufferAddLit(buf, "<value>dir</value>\n"); - if (poolOptions->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) - virBufferAddLit(buf, "<value>adapter</value>\n"); - if (poolOptions->flags & VIR_STORAGE_POOL_SOURCE_NAME) - virBufferAddLit(buf, "<value>name</value>\n"); - if (poolOptions->flags & VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN) - virBufferAddLit(buf, "<value>initiator</value>\n"); - if (poolOptions->flags & VIR_STORAGE_POOL_SOURCE_NETWORK) - virBufferAddLit(buf, "<value>network</value>\n"); - - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</enum>\n"); - } - virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</poolOptions>\n"); return 0; diff --git a/tests/storagepoolcapsschemadata/poolcaps-fs.xml b/tests/storagepoolcapsschemadata/poolcaps-fs.xml index 0e15af0607..6513ea621a 100644 --- a/tests/storagepoolcapsschemadata/poolcaps-fs.xml +++ b/tests/storagepoolcapsschemadata/poolcaps-fs.xml @@ -41,9 +41,6 @@ <value>xfs</value> <value>ocfs2</value> </enum> - <enum name='requiredSourceElements'> - <value>device</value> - </enum> </poolOptions> <volOptions> <defaultFormat type='raw'/> @@ -77,10 +74,6 @@ <value>glusterfs</value> <value>cifs</value> </enum> - <enum name='requiredSourceElements'> - <value>host</value> - <value>dir</value> - </enum> </poolOptions> <volOptions> <defaultFormat type='raw'/> @@ -112,10 +105,6 @@ <value>unknown</value> <value>lvm2</value> </enum> - <enum name='requiredSourceElements'> - <value>device</value> - <value>name</value> - </enum> </poolOptions> </pool> <pool type='disk' supported='no'> @@ -132,9 +121,6 @@ <value>sun</value> <value>lvm2</value> </enum> - <enum name='requiredSourceElements'> - <value>device</value> - </enum> </poolOptions> <volOptions> <defaultFormat type='none'/> @@ -151,60 +137,18 @@ </volOptions> </pool> <pool type='iscsi' supported='no'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>device</value> - <value>initiator</value> - </enum> - </poolOptions> </pool> <pool type='iscsi-direct' supported='no'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>device</value> - <value>initiator</value> - <value>network</value> - </enum> - </poolOptions> </pool> <pool type='scsi' supported='no'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>adapter</value> - </enum> - </poolOptions> </pool> <pool type='mpath' supported='no'> </pool> <pool type='rbd' supported='no'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>name</value> - <value>network</value> - </enum> - </poolOptions> </pool> <pool type='sheepdog' supported='no'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>name</value> - <value>network</value> - </enum> - </poolOptions> </pool> <pool type='gluster' supported='no'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>dir</value> - <value>name</value> - <value>network</value> - </enum> - </poolOptions> <volOptions> <defaultFormat type='raw'/> <enum name='targetFormatType'> @@ -229,19 +173,8 @@ </volOptions> </pool> <pool type='zfs' supported='no'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>device</value> - <value>name</value> - </enum> - </poolOptions> </pool> <pool type='vstorage' supported='no'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>name</value> - </enum> - </poolOptions> <volOptions> <defaultFormat type='raw'/> <enum name='targetFormatType'> diff --git a/tests/storagepoolcapsschemadata/poolcaps-full.xml b/tests/storagepoolcapsschemadata/poolcaps-full.xml index 0bb3faf04e..32003dd608 100644 --- a/tests/storagepoolcapsschemadata/poolcaps-full.xml +++ b/tests/storagepoolcapsschemadata/poolcaps-full.xml @@ -41,9 +41,6 @@ <value>xfs</value> <value>ocfs2</value> </enum> - <enum name='requiredSourceElements'> - <value>device</value> - </enum> </poolOptions> <volOptions> <defaultFormat type='raw'/> @@ -77,10 +74,6 @@ <value>glusterfs</value> <value>cifs</value> </enum> - <enum name='requiredSourceElements'> - <value>host</value> - <value>dir</value> - </enum> </poolOptions> <volOptions> <defaultFormat type='raw'/> @@ -112,10 +105,6 @@ <value>unknown</value> <value>lvm2</value> </enum> - <enum name='requiredSourceElements'> - <value>device</value> - <value>name</value> - </enum> </poolOptions> </pool> <pool type='disk' supported='yes'> @@ -132,9 +121,6 @@ <value>sun</value> <value>lvm2</value> </enum> - <enum name='requiredSourceElements'> - <value>device</value> - </enum> </poolOptions> <volOptions> <defaultFormat type='none'/> @@ -151,60 +137,18 @@ </volOptions> </pool> <pool type='iscsi' supported='yes'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>device</value> - <value>initiator</value> - </enum> - </poolOptions> </pool> <pool type='iscsi-direct' supported='yes'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>device</value> - <value>initiator</value> - <value>network</value> - </enum> - </poolOptions> </pool> <pool type='scsi' supported='yes'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>adapter</value> - </enum> - </poolOptions> </pool> <pool type='mpath' supported='yes'> </pool> <pool type='rbd' supported='yes'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>name</value> - <value>network</value> - </enum> - </poolOptions> </pool> <pool type='sheepdog' supported='yes'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>name</value> - <value>network</value> - </enum> - </poolOptions> </pool> <pool type='gluster' supported='yes'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>host</value> - <value>dir</value> - <value>name</value> - <value>network</value> - </enum> - </poolOptions> <volOptions> <defaultFormat type='raw'/> <enum name='targetFormatType'> @@ -229,19 +173,8 @@ </volOptions> </pool> <pool type='zfs' supported='yes'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>device</value> - <value>name</value> - </enum> - </poolOptions> </pool> <pool type='vstorage' supported='yes'> - <poolOptions> - <enum name='requiredSourceElements'> - <value>name</value> - </enum> - </poolOptions> <volOptions> <defaultFormat type='raw'/> <enum name='targetFormatType'> -- 2.20.1
On Wed, Mar 06, 2019 at 19:16:48 +0100, Pavel Hrdina wrote:
Capabilities should not duplicate data that are obvious from our documentation and will not change with different QEMU binaries or the way how we compile libvirt.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
I agree that this is not very useful. poolOptions->flags is more of a hint for the parser to do validation (since storage pools don't have postparse/validate callbacks). Many new features which would be worth detecting via queryable capabilities will not show up here, thus a different mechanism would be better suited. ACK
participants (2)
-
Pavel Hrdina -
Peter Krempa