12:16 p.m.
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
12:16 p.m.
New subject: [libvirt] [PATCH 1/3] tests: use virTestCompareToFile in storagepoolcapstest
This will allow to use VIR_TEST_REGENERATE_OUTPUT.
Signed-off-by: Pavel Hrdina <phrdina(a)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
2:08 a.m.
New subject: [libvirt] [PATCH 1/3] tests: use virTestCompareToFile in storagepoolcapstest
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(a)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.
12:16 p.m.
New subject: [libvirt] [PATCH 2/3] tests: invert the return logic in storagepoolcapstest
This way if the first test "full" fails we will run the second test as
well.
Signed-off-by: Pavel Hrdina <phrdina(a)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
2:11 a.m.
New subject: [libvirt] [PATCH 2/3] tests: invert the return logic in storagepoolcapstest
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(a)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.
3:56 a.m.
New subject: [libvirt] [PATCH 2/3] tests: invert the return logic in storagepoolcapstest
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(a)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
12:16 p.m.
New subject: [libvirt] [PATCH 3/3] caps: drop requiredSourceElements from storage pool capabilities
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(a)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
2:21 a.m.
New subject: [libvirt] [PATCH 3/3] caps: drop requiredSourceElements from storage pool capabilities
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(a)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
2087
days inactive
2088
days old
7 comments
2 participants
participants (2)
-
Pavel Hrdina -
Peter Krempa