3:57 a.m.
See 2/6 for explanation.
Michal Prívozník (6):
nodedevmdevctltest: Rename mdevctl-list-empty test case
nodeDeviceParseMdevctlJSON: Accept empty string
nodedevmdevctltest: Introduce a test case for empty mdevctl output
node_device_driver: Deduplicate mediated devices listing
virMdevctlList: Don't check for !output
virjsontest: Introduce a test case for an empty array
src/node_device/node_device_driver.c | 40 ++++++-------------
.../mdevctl-list-empty-array.json | 1 +
.../mdevctl-list-empty-array.out.xml | 0
.../mdevctl-list-empty.json | 1 -
tests/nodedevmdevctltest.c | 1 +
tests/virjsontest.c | 1 +
6 files changed, 16 insertions(+), 28 deletions(-)
create mode 100644 tests/nodedevmdevctldata/mdevctl-list-empty-array.json
create mode 100644 tests/nodedevmdevctldata/mdevctl-list-empty-array.out.xml
--
2.41.0
3:57 a.m.
New subject: [PATCH 1/6] nodedevmdevctltest: Rename mdevctl-list-empty test case
The mdevctl-list-empty test case is there to test whether an
empty JSON array "[]" is handled correctly by mdevctl handling
code. Well, mdevctl can output both, an empty JSON array or no
output at all.
Therefore, rename "mdevctl-list-empty" test case to
"mdevctl-list-empty-array" which is more descriptive and also
frees up slot for actual empty output (handled in next commits).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
.../{mdevctl-list-empty.json => mdevctl-list-empty-array.json} | 0
...vctl-list-empty.out.xml => mdevctl-list-empty-array.out.xml} | 0
tests/nodedevmdevctltest.c | 2 +-
3 files changed, 1 insertion(+), 1 deletion(-)
rename tests/nodedevmdevctldata/{mdevctl-list-empty.json =>
mdevctl-list-empty-array.json} (100%)
rename tests/nodedevmdevctldata/{mdevctl-list-empty.out.xml =>
mdevctl-list-empty-array.out.xml} (100%)
diff --git a/tests/nodedevmdevctldata/mdevctl-list-empty.json
b/tests/nodedevmdevctldata/mdevctl-list-empty-array.json
similarity index 100%
rename from tests/nodedevmdevctldata/mdevctl-list-empty.json
rename to tests/nodedevmdevctldata/mdevctl-list-empty-array.json
diff --git a/tests/nodedevmdevctldata/mdevctl-list-empty.out.xml
b/tests/nodedevmdevctldata/mdevctl-list-empty-array.out.xml
similarity index 100%
rename from tests/nodedevmdevctldata/mdevctl-list-empty.out.xml
rename to tests/nodedevmdevctldata/mdevctl-list-empty-array.out.xml
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index 4dc524b5a5..c04b05c417 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -470,7 +470,7 @@ mymain(void)
DO_TEST_LIST_DEFINED();
- DO_TEST_PARSE_JSON("mdevctl-list-empty");
+ DO_TEST_PARSE_JSON("mdevctl-list-empty-array");
DO_TEST_PARSE_JSON("mdevctl-list-multiple");
DO_TEST_DEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
--
2.41.0
3:57 a.m.
New subject: [PATCH 2/6] nodeDeviceParseMdevctlJSON: Accept empty string
It is possible for 'mdevctl' to output nothing, an empty string
(e.g. when no mediated devices are defined on the host). What is
weird is that when passing '--defined' then 'mdevctl' outputs an
empty JSON array instead. Nevertheless, we should accept both and
threat them the same, i.e. as no mediated devices.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/523
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/node_device/node_device_driver.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 2ef9197adc..593bc64e25 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -43,6 +43,7 @@
#include "virutil.h"
#include "vircommand.h"
#include "virlog.h"
+#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_NODEDEV
@@ -1176,6 +1177,12 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
size_t j;
virJSONValue *obj;
+ if (virStringIsEmpty(jsonstring)) {
+ VIR_DEBUG("mdevctl has no defined mediated devices");
+ *devs = NULL;
+ return 0;
+ }
+
json_devicelist = virJSONValueFromString(jsonstring);
if (!json_devicelist || !virJSONValueIsArray(json_devicelist)) {
--
2.41.0
6:25 a.m.
New subject: [PATCH 2/6] nodeDeviceParseMdevctlJSON: Accept empty string
On Thu, Aug 24, 2023 at 10:57 AM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
It is possible for 'mdevctl' to output nothing, an empty
string
(e.g. when no mediated devices are defined on the host). What is
weird is that when passing '--defined' then 'mdevctl' outputs an
empty JSON array instead. Nevertheless, we should accept both and
threat them the same, i.e. as no mediated devices.
*treat
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/523
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/node_device/node_device_driver.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index 2ef9197adc..593bc64e25 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -43,6 +43,7 @@
#include "virutil.h"
#include "vircommand.h"
#include "virlog.h"
+#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_NODEDEV
@@ -1176,6 +1177,12 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
size_t j;
virJSONValue *obj;
+ if (virStringIsEmpty(jsonstring)) {
+ VIR_DEBUG("mdevctl has no defined mediated devices");
+ *devs = NULL;
+ return 0;
+ }
+
json_devicelist = virJSONValueFromString(jsonstring);
if (!json_devicelist || !virJSONValueIsArray(json_devicelist)) {
--
2.41.0
Reviewed-by: Kristina Hanicova <khanicov(a)redhat.com>
Kristina
3:57 a.m.
New subject: [PATCH 3/6] nodedevmdevctltest: Introduce a test case for empty mdevctl output
As explained earlier, 'mdevctl' can output nothing. Add a test
case to nodedevmdevctltest which covers this situation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/nodedevmdevctldata/mdevctl-list-empty.json | 0
tests/nodedevmdevctldata/mdevctl-list-empty.out.xml | 0
tests/nodedevmdevctltest.c | 1 +
3 files changed, 1 insertion(+)
create mode 100644 tests/nodedevmdevctldata/mdevctl-list-empty.json
create mode 100644 tests/nodedevmdevctldata/mdevctl-list-empty.out.xml
diff --git a/tests/nodedevmdevctldata/mdevctl-list-empty.json
b/tests/nodedevmdevctldata/mdevctl-list-empty.json
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/nodedevmdevctldata/mdevctl-list-empty.out.xml
b/tests/nodedevmdevctldata/mdevctl-list-empty.out.xml
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index c04b05c417..e403328e5a 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -470,6 +470,7 @@ mymain(void)
DO_TEST_LIST_DEFINED();
+ DO_TEST_PARSE_JSON("mdevctl-list-empty");
DO_TEST_PARSE_JSON("mdevctl-list-empty-array");
DO_TEST_PARSE_JSON("mdevctl-list-multiple");
--
2.41.0
3:57 a.m.
New subject: [PATCH 4/6] node_device_driver: Deduplicate mediated devices listing
We have virMdevctlListDefined() to list defined mdevs, and
virMdevctlListActive() to list active mdevs. Both have the same
body except for one boolean argument passed to
nodeDeviceGetMdevctlListCommand(). Join the two functions under
virMdevctlList() name and introduce @defined argument that is
then just passed to the cmd line builder function.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/node_device/node_device_driver.c | 30 ++++++----------------------
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 593bc64e25..ac50c96837 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1636,32 +1636,14 @@ nodeDeviceGenerateName(virNodeDeviceDef *def,
static int
-virMdevctlListDefined(virNodeDeviceDef ***devs, char **errmsg)
+virMdevctlList(bool defined,
+ virNodeDeviceDef ***devs,
+ char **errmsg)
{
int status;
g_autofree char *output = NULL;
g_autofree char *errbuf = NULL;
- g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output,
&errbuf);
-
- if (virCommandRun(cmd, &status) < 0 || status != 0) {
- *errmsg = g_steal_pointer(&errbuf);
- return -1;
- }
-
- if (!output)
- return -1;
-
- return nodeDeviceParseMdevctlJSON(output, devs);
-}
-
-
-static int
-virMdevctlListActive(virNodeDeviceDef ***devs, char **errmsg)
-{
- int status;
- g_autofree char *output = NULL;
- g_autofree char *errbuf = NULL;
- g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(false, &output,
&errbuf);
+ g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(defined, &output,
&errbuf);
if (virCommandRun(cmd, &status) < 0 || status != 0) {
*errmsg = g_steal_pointer(&errbuf);
@@ -1750,7 +1732,7 @@ nodeDeviceUpdateMediatedDevices(void)
return 0;
}
- if ((data.ndefs = virMdevctlListDefined(&defs, &errmsg)) < 0) {
+ if ((data.ndefs = virMdevctlList(true, &defs, &errmsg)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to query mdevs from mdevctl: %1$s"), errmsg);
return -1;
@@ -1767,7 +1749,7 @@ nodeDeviceUpdateMediatedDevices(void)
return -1;
/* Update active/transient mdev devices */
- if ((act_ndefs = virMdevctlListActive(&act_defs, &errmsg)) < 0) {
+ if ((act_ndefs = virMdevctlList(false, &act_defs, &errmsg)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to query mdevs from mdevctl: %1$s"), errmsg);
return -1;
--
2.41.0
3:57 a.m.
New subject: [PATCH 5/6] virMdevctlList: Don't check for !output
After 'mdevctl' was ran, its stdout is captured in @output which
is then compared against NULL and if it is NULL a negative value
is returned (to indicate error to the caller). But this is
effectively a dead code, because virCommand (specifically
virCommandProcessIO()) makes sure both stdout and stderr buffers
are properly '\0' terminated. Therefore, this can never evaluate
to true. Also, if there really is no output from 'mdevctl' (which
was handled in one of earlier commits, but let just assume it
wasn't), then we should not error out and treat such scenario as
'no mdevs defined/active'.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/node_device/node_device_driver.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index ac50c96837..a59cd0875d 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1650,9 +1650,6 @@ virMdevctlList(bool defined,
return -1;
}
- if (!output)
- return -1;
-
return nodeDeviceParseMdevctlJSON(output, devs);
}
--
2.41.0
3:57 a.m.
New subject: [PATCH 6/6] virjsontest: Introduce a test case for an empty array
Previous commits were all about empty strings and empty JSON
arrays. Introduce a test case for "[]" to make sure we pare it
correctly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virjsontest.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/virjsontest.c b/tests/virjsontest.c
index 294889a795..6b6a64d3d3 100644
--- a/tests/virjsontest.c
+++ b/tests/virjsontest.c
@@ -553,6 +553,7 @@ mymain(void)
DO_TEST_PARSE("integer", "1", NULL);
DO_TEST_PARSE("boolean", "true", NULL);
DO_TEST_PARSE("null", "null", NULL);
+ DO_TEST_PARSE("[]", "[]", NULL);
DO_TEST_PARSE("escaping symbols",
"[\"\\\"\\t\\n\\\\\"]", NULL);
DO_TEST_PARSE("escaped strings",
"[\"{\\\"blurb\\\":\\\"test\\\"}\"]", NULL);
--
2.41.0
6:42 a.m.
On Thu, Aug 24, 2023 at 10:57 AM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
See 2/6 for explanation.
Michal Prívozník (6):
nodedevmdevctltest: Rename mdevctl-list-empty test case
nodeDeviceParseMdevctlJSON: Accept empty string
nodedevmdevctltest: Introduce a test case for empty mdevctl output
node_device_driver: Deduplicate mediated devices listing
virMdevctlList: Don't check for !output
virjsontest: Introduce a test case for an empty array
src/node_device/node_device_driver.c | 40 ++++++-------------
.../mdevctl-list-empty-array.json | 1 +
.../mdevctl-list-empty-array.out.xml | 0
.../mdevctl-list-empty.json | 1 -
tests/nodedevmdevctltest.c | 1 +
tests/virjsontest.c | 1 +
6 files changed, 16 insertions(+), 28 deletions(-)
create mode 100644 tests/nodedevmdevctldata/mdevctl-list-empty-array.json
create mode 100644
tests/nodedevmdevctldata/mdevctl-list-empty-array.out.xml
Reviewed-by: Kristina Hanicova <khanicov(a)redhat.com>
Kristina
409
days inactive
409
days old
8 comments
2 participants
participants (2)
-
Kristina Hanicova -
Michal Privoznik