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

2021-07-15 Thread Boris Fiuczynski 

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


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

2021-07-15 Thread Shalini Chellathurai Saroja 



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


Hello Boris,

I tested the patch, it works fine.

Reviewed-by: Shalini Chellathurai Saroja 

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

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

2021-07-15 Thread Martin Kletzander 

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


signature.asc
Description: PGP signature

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

2021-07-14 Thread Boris Fiuczynski 
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 
---
 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 00..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 00..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