On Wed, Feb 03, 2021 at 11:38:49AM -0600, Jonathon Jongsma wrote:
This function will parse the list of mediated devices that are
returned
by mdevctl and convert it into our internal node device representation.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 145 ++++++++++++++++++
src/node_device/node_device_driver.h | 4 +
.../mdevctl-list-multiple.json | 59 +++++++
.../mdevctl-list-multiple.out.xml | 39 +++++
tests/nodedevmdevctltest.c | 56 ++++++-
5 files changed, 301 insertions(+), 2 deletions(-)
create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.json
create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 6143459618..fc5ac1d27e 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -853,6 +853,151 @@ virMdevctlStop(virNodeDeviceDefPtr def)
}
+static void mdevGenerateDeviceName(virNodeDeviceDefPtr dev)
+{
+ nodeDeviceGenerateName(dev, "mdev", dev->caps->data.mdev.uuid,
NULL);
+}
+
+
+static virNodeDeviceDefPtr
+nodeDeviceParseMdevctlChildDevice(const char *parent,
+ virJSONValuePtr json)
+{
+ virNodeDevCapMdevPtr mdev;
+ const char *uuid;
+ virJSONValuePtr props, attrs;
^This was the other "One declaration per line" place I could not remember
during v3 review.
...
+int
+nodeDeviceParseMdevctlJSON(const char *jsonstring,
+ virNodeDeviceDefPtr **devs)
+{
+ int n;
+ g_autoptr(virJSONValue) json_devicelist = NULL;
+ virNodeDeviceDefPtr *outdevs = NULL;
+ size_t noutdevs = 0;
+ size_t i;
+ size_t j;
+
+ json_devicelist = virJSONValueFromString(jsonstring);
+
+ if (!json_devicelist || !virJSONValueIsArray(json_devicelist)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("JSON response contains no devices"));
I'd make it even more specific - "mdevctl JSON response..."
+ goto error;
+ }
+
+ n = virJSONValueArraySize(json_devicelist);
+
+ for (i = 0; i < n; i++) {
+ virJSONValuePtr obj = virJSONValueArrayGet(json_devicelist, i);
+ const char *parent;
+ virJSONValuePtr child_array;
+ int nchildren;
+
+ if (!virJSONValueIsObject(obj)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Parent device is not an object"));
+ goto error;
+ }
+
+ /* mdevctl returns an array of objects. Each object is a parent device
+ * object containing a single key-value pair which maps from the name
+ * of the parent device to an array of child devices */
+ if (virJSONValueObjectKeysNumber(obj) != 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unexpected format for parent device object"));
+ goto error;
+ }
+
+ parent = virJSONValueObjectGetKey(obj, 0);
+ child_array = virJSONValueObjectGetValue(obj, 0);
+
+ if (!virJSONValueIsArray(child_array)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Child list is not an array"));
"Parent device's JSON object data is not an array" - or something along
those
lines
+ goto error;
+ }
+
+ nchildren = virJSONValueArraySize(child_array);
+
+ for (j = 0; j < nchildren; j++) {
+ g_autoptr(virNodeDeviceDef) child = NULL;
+ virJSONValuePtr child_obj = virJSONValueArrayGet(child_array, j);
+
+ if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to parse child device"));
"Unable to parse mdev child device"
+ goto error;
+ }
+
+ if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to append child device to list"));
APPEND_ELEMENT will either report an "Out of bounds" error or abort on
allocation failure, so please drop ^this virReportError.
+ goto error;
+ }
+ }
+ }
+
+ *devs = outdevs;
+ return noutdevs;
+
+ error:
+ for (i = 0; i < noutdevs; i++)
+ virNodeDeviceDefFree(outdevs[i]);
+ VIR_FREE(outdevs);
+ return -1;
+}
+
+
int
nodeDeviceDestroy(virNodeDevicePtr device)
{
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index 02baf56dab..2a162513c0 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -119,6 +119,10 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
virCommandPtr
nodeDeviceGetMdevctlStopCommand(const char *uuid);
+int
+nodeDeviceParseMdevctlJSON(const char *jsonstring,
+ virNodeDeviceDefPtr **devs);
+
void
nodeDeviceGenerateName(virNodeDeviceDefPtr def,
const char *subsystem,
diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.json
b/tests/nodedevmdevctldata/mdevctl-list-multiple.json
new file mode 100644
index 0000000000..eefcd90c62
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.json
@@ -0,0 +1,59 @@
+[
+ {
+ "0000:00:02.0": [
+ {
+ "200f228a-c80a-4d50-bfb7-f5a0e4e34045": {
+ "mdev_type": "i915-GVTg_V5_4",
+ "start": "manual"
+ }
+ },
+ {
+ "de807ffc-1923-4d5f-b6c9-b20ecebc6d4b": {
+ "mdev_type": "i915-GVTg_V5_4",
+ "start": "auto"
+ }
+ },
+ {
+ "435722ea-5f43-468a-874f-da34f1217f13": {
+ "mdev_type": "i915-GVTg_V5_8",
+ "start": "manual",
+ "attrs": [
+ {
+ "testattr": "42"
+ }
+ ]
+ }
+ }
+ ]
+ },
+ {
+ "matrix": [
+ { "783e6dbb-ea0e-411f-94e2-717eaad438bf": {
+ "mdev_type": "vfio_ap-passthrough",
+ "start": "manual",
+ "attrs": [
+ {
+ "assign_adapter": "5"
+ },
+ {
+ "assign_adapter": "6"
+ },
I'd still like to know why there are 2 assign_adapter and 2 assign_domain
attributes here, I'm simply confused what the outcome should be.
+ {
+ "assign_domain": "0xab"
+ },
+ {
+ "assign_control_domain": "0xab"
+ },
+ {
+ "assign_domain": "4"
+ },
+ {
+ "assign_control_domain": "4"
+ }
+ ]
+ }
+ }
+ ]
+ }
+]
+
...
+
<name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name>
+ <parent>matrix</parent>
+ <capability type='mdev'>
+ <type id='vfio_ap-passthrough'/>
+ <iommuGroup number='0'/>
+ <attr name='assign_adapter' value='5'/>
+ <attr name='assign_adapter' value='6'/>
+ <attr name='assign_domain' value='0xab'/>
+ <attr name='assign_control_domain' value='0xab'/>
+ <attr name='assign_domain' value='4'/>
+ <attr name='assign_control_domain' value='4'/>
Here too I'd like to hear an opinion (since v3) on naming the attributes in
such way that they correspond to the respective elements: ap-adapter,
ap-domain, etc. This naming is very unintuitive if not documented properly;
unless there's an internal reason why they have to be named
"assign_control",
etc.
Erik