[v2] nodedev: fix internal error when no defined mdevs exist

[PATCH v2] nodedev: fix internal error when no defined mdevs exist

Posted by Boris Fiuczynski 2 years, 9 months ago

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@linux.ibm.com>
---
 src/node_device/node_device_driver.c                | 5 +++++
 tests/nodedevmdevctldata/mdevctl-list-empty.json    | 1 +
 tests/nodedevmdevctldata/mdevctl-list-empty.out.xml | 0
 tests/nodedevmdevctltest.c                          | 1 +
 4 files changed, 7 insertions(+)
 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..b16b05f9b2 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1120,6 +1120,11 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
         goto error;
     }
 
+    if (virJSONValueArraySize(json_devicelist) == 0) {
+        *devs = outdevs;
+        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");
-- 
2.31.1

Re: [PATCH v2] nodedev: fix internal error when no defined mdevs exist

Posted by Martin Kletzander 2 years, 9 months ago

On Wed, Jul 14, 2021 at 06:40:57PM +0200, Boris Fiuczynski 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
>

causes

> 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@linux.ibm.com>
>---
> src/node_device/node_device_driver.c                | 5 +++++
> tests/nodedevmdevctldata/mdevctl-list-empty.json    | 1 +
> tests/nodedevmdevctldata/mdevctl-list-empty.out.xml | 0
> tests/nodedevmdevctltest.c                          | 1 +
> 4 files changed, 7 insertions(+)
> 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..b16b05f9b2 100644
>--- a/src/node_device/node_device_driver.c
>+++ b/src/node_device/node_device_driver.c
>@@ -1120,6 +1120,11 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
>         goto error;
>     }
>
>+    if (virJSONValueArraySize(json_devicelist) == 0) {
>+        *devs = outdevs;
>+        return 0;

It would be more clear if you just did:

   *devs = NULL;

And I, personally, would also add a VIR_DEBUG here.

Other than that

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Re: [PATCH v2] nodedev: fix internal error when no defined mdevs exist

Posted by Boris Fiuczynski 2 years, 9 months ago

On 7/15/21 2:16 PM, Martin Kletzander wrote:
> On Wed, Jul 14, 2021 at 06:40:57PM +0200, Boris Fiuczynski 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
>>
> 
> causes
> 
>> 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@linux.ibm.com>
>> ---
>> src/node_device/node_device_driver.c                | 5 +++++
>> tests/nodedevmdevctldata/mdevctl-list-empty.json    | 1 +
>> tests/nodedevmdevctldata/mdevctl-list-empty.out.xml | 0
>> tests/nodedevmdevctltest.c                          | 1 +
>> 4 files changed, 7 insertions(+)
>> 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..b16b05f9b2 100644
>> --- a/src/node_device/node_device_driver.c
>> +++ b/src/node_device/node_device_driver.c
>> @@ -1120,6 +1120,11 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
>>         goto error;
>>     }
>>
>> +    if (virJSONValueArraySize(json_devicelist) == 0) {
>> +        *devs = outdevs;
>> +        return 0;
> 
> It would be more clear if you just did:
> 
>    *devs = NULL;

Yeah, sure.

> 
> And I, personally, would also add a VIR_DEBUG here.

I considered the scenario that mdevctl has no mediated device 
definitions stored a very normal case and not worth a debug message as 
the rest of the code does not report debug data if mediated device 
definitions exist.

> 
> Other than that
> 
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Thanks for reviewing.
Do you want me to send a v3 with the changes?

-- 
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

Re: [PATCH v2] nodedev: fix internal error when no defined mdevs exist

Posted by Shalini Chellathurai Saroja 2 years, 9 months ago

On 7/14/21 6:40 PM, Boris Fiuczynski 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@linux.ibm.com>

Hello Boris,

I tested the patch, it works fine.

Reviewed-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>

-- 
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294