Re: [PATCH] nodedev: fix error when no defined mdev exist

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On 7/9/21 6:25 PM, Jonathon Jongsma wrote:

On Fri,  9 Jul 2021 15:39:47 +0200
Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> wrote:


Commit e9b534905f4 introduced an error when parsing an empty list
returned from mdevctl.

This occurs e.g. if nodedev-undefine is used to undefine the last
defined mdev which cuases the following error messages

  libvirtd[33143]: internal error: Unexpected format for mdevctl
response libvirtd[33143]: internal error: failed to query mdevs from
mdevctl: libvirtd[33143]: mdevctl failed to updated mediated devices

Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
---
  src/node_device/node_device_driver.c          | 68
++++++++++--------- .../mdevctl-list-empty.json                   |
1 + .../mdevctl-list-empty.out.xml                |  0
  tests/nodedevmdevctltest.c                    |  1 +
  4 files changed, 37 insertions(+), 33 deletions(-)
  create mode 100644 tests/nodedevmdevctldata/mdevctl-list-empty.json
  create mode 100644
tests/nodedevmdevctldata/mdevctl-list-empty.out.xml

diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c index b4dd57e5f4..31525f2312
100644 --- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1123,51 +1123,53 @@ nodeDeviceParseMdevctlJSON(const char
*jsonstring, /* mdevctl list --dumpjson produces an output that is an
array that
       * contains only a single object which contains a property for
each parent
       * device */
-    if (virJSONValueArraySize(json_devicelist) != 1) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Unexpected format for mdevctl response"));
-        goto error;
-    }
-
-    obj = virJSONValueArrayGet(json_devicelist, 0);
-
-    if (!virJSONValueIsObject(obj)) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("device list is not an object"));
-        goto error;
-    }
-
-    n = virJSONValueObjectKeysNumber(obj);
-    for (i = 0; i < n; i++) {
-        const char *parent;
-        virJSONValue *child_array;
-        int nchildren;
+    if (virJSONValueArraySize(json_devicelist) > 0) {
+        if (virJSONValueArraySize(json_devicelist) != 1) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Unexpected format for mdevctl
response"));
+            goto error;
+        }
- /* The key of each object property is the name of a parent 
device
-         * which maps to an array of child devices */
-        parent = virJSONValueObjectGetKey(obj, i);
-        child_array = virJSONValueObjectGetValue(obj, i);
+        obj = virJSONValueArrayGet(json_devicelist, 0);
- if (!virJSONValueIsArray(child_array)) { 
+        if (!virJSONValueIsObject(obj)) {
              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                           _("Parent device's JSON object data is
not an array"));
+                           _("device list is not an object"));
              goto error;
          }
- nchildren = virJSONValueArraySize(child_array); 
+        n = virJSONValueObjectKeysNumber(obj);
+        for (i = 0; i < n; i++) {
+            const char *parent;
+            virJSONValue *child_array;
+            int nchildren;
- for (j = 0; j < nchildren; j++) { 
-            g_autoptr(virNodeDeviceDef) child = NULL;
-            virJSONValue *child_obj =
virJSONValueArrayGet(child_array, j);
+            /* The key of each object property is the name of a
parent device
+             * which maps to an array of child devices */
+            parent = virJSONValueObjectGetKey(obj, i);
+            child_array = virJSONValueObjectGetValue(obj, i);
- if (!(child = nodeDeviceParseMdevctlChildDevice(parent, 
child_obj))) {
+            if (!virJSONValueIsArray(child_array)) {
                  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("Unable to parse child device"));
+                               _("Parent device's JSON object data
is not an array")); goto error;
              }
- if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0) 
-                goto error;
+            nchildren = virJSONValueArraySize(child_array);
+
+            for (j = 0; j < nchildren; j++) {
+                g_autoptr(virNodeDeviceDef) child = NULL;
+                virJSONValue *child_obj =
virJSONValueArrayGet(child_array, j); +
+                if (!(child =
nodeDeviceParseMdevctlChildDevice(parent, child_obj))) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                                   _("Unable to parse child
device"));
+                    goto error;
+                }
+
+                if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0)
+                    goto error;
+            }
          }
      }
diff --git a/tests/nodedevmdevctldata/mdevctl-list-empty.json 
b/tests/nodedevmdevctldata/mdevctl-list-empty.json new file mode
100644 index 0000000000..fe51488c70
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdevctl-list-empty.json
@@ -0,0 +1 @@
+[]
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 8ba1d2da70..e246de4d87 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -360,6 +360,7 @@ mymain(void)
DO_TEST_LIST_DEFINED(); + DO_TEST_PARSE_JSON("mdevctl-list-empty"); 
      DO_TEST_PARSE_JSON("mdevctl-list-multiple");
DO_TEST_DEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); 


Thanks for catching this. I'd prefer to simplify this patch by
returning early if there are no elements in the array. That reduces the
diff to about 5 total lines changed, including the new test:

diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c index b4dd57e5f4..f63a65e26a
100644 --- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1120,6 +1120,9 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
          goto error;
      }
+ if (virJSONValueArraySize(json_devicelist) == 0) 
+        return 0;
This was kind of what I did first, too, but I thought I would require to also set not only the length of the array but also the array itself. 
That's why I ended up with this.

    if (virJSONValueArraySize(json_devicelist) == 0) {
        *devs = outdevs;
        return 0;
    }
and since that matches exactly the already existing return I wrapped the if-statement around the existing code block... causing a horribly looking diff. But actually there are just two added lines: 

    if (virJSONValueArraySize(json_devicelist) > 0) {
        {old code with 4 additional leading blanks for the indentation}
    }


+
      /* mdevctl list --dumpjson produces an output that is an array that
       * contains only a single object which contains a property for
each parent
       * device */
diff --git a/tests/nodedevmdevctldata/mdevctl-list-empty.json
b/tests/nodedevmdevctldata/mdevctl-list-empty.json new file mode 100644
index 0000000000..fe51488c70
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdevctl-list-empty.json
@@ -0,0 +1 @@
+[]
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 8ba1d2da70..e246de4d87 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -360,6 +360,7 @@ mymain(void)
DO_TEST_LIST_DEFINED(); + DO_TEST_PARSE_JSON("mdevctl-list-empty"); 
      DO_TEST_PARSE_JSON("mdevctl-list-multiple");
DO_TEST_DEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); 




--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux