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

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

 



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;
+
     /* 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");




[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