[PATCH 0/6] Fix one corner case when parsing 'mdevctl' output
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
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@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
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@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
On Thu, Aug 24, 2023 at 10:57 AM Michal Privoznik <mprivozn@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@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@redhat.com> Kristina
As explained earlier, 'mdevctl' can output nothing. Add a test case to nodedevmdevctltest which covers this situation. Signed-off-by: Michal Privoznik <mprivozn@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
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@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
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@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
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@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
On Thu, Aug 24, 2023 at 10:57 AM Michal Privoznik <mprivozn@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@redhat.com> Kristina
participants (2)
-
Kristina Hanicova -
Michal Privoznik