6:52 a.m.
On Tue, Aug 18, 2020 at 09:47:55AM -0500, Jonathon Jongsma wrote:
This adds some internal API to query for persistent mediated devices
that are defined by mdevctl. Following commits will make use of this
information. This just provides the infrastructure and tests for this
feature. One test verifies that we are executing mdevctl with the proper
arguments, and other tests verify that we can parse the returned JSON
and convert it to our own internal node device representations.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 156 ++++++++++++++++++
src/node_device/node_device_driver.h | 13 ++
src/node_device/node_device_udev.c | 19 +--
.../mdevctl-list-defined.argv | 1 +
.../mdevctl-list-multiple-parents.json | 59 +++++++
.../mdevctl-list-multiple-parents.out.xml | 39 +++++
.../mdevctl-list-multiple.json | 59 +++++++
.../mdevctl-list-multiple.out.xml | 39 +++++
I see literally zero difference between the respective file pairs ^above. Was
that intentional or they should have tested something else?
.../mdevctl-list-single-noattr.json | 11 ++
.../mdevctl-list-single-noattr.out.xml | 8 +
.../mdevctl-list-single.json | 31 ++++
.../mdevctl-list-single.out.xml | 14 ++
I'm all for testing, but I feel like ^these single device use cases are both
covered with the multiple ones introduced above, since those include
devices both with attributes as well as without them.
...
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index f766fd9f32..3b042e9a45 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -812,6 +812,137 @@ virMdevctlStop(virNodeDeviceDefPtr def)
}
+virCommandPtr
+nodeDeviceGetMdevctlListCommand(bool defined,
+ char **output)
+{
+ virCommandPtr cmd = virCommandNewArgList(MDEVCTL,
+ "list",
+ "--dumpjson",
+ NULL);
+
+ if (defined)
+ virCommandAddArg(cmd, "--defined");
+
+ virCommandSetOutputBuffer(cmd, output);
+
+ return cmd;
+}
+
+
+static void mdevGenerateDeviceName(virNodeDeviceDefPtr dev)
^this name generator code movement should IMO be a standalone patch.
+{
+ nodeDeviceGenerateName(dev, "mdev", dev->caps->data.mdev.uuid,
NULL);
+}
+
^2 blank lines between functions.
+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, j, k, m;
+
+ json_devicelist = virJSONValueFromString(jsonstring);
+
+ if (!json_devicelist)
+ goto parsefailure;
+
+ if (!virJSONValueIsArray(json_devicelist))
+ goto parsefailure;
+
+ n = virJSONValueArraySize(json_devicelist);
given the following JSON:
[
{
"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"
},
{
"assign_domain": "0xab"
},
{
"assign_control_domain": "0xab"
},
{
"assign_domain": "4"
},
{
"assign_control_domain": "4"
}
]
}
}
]
}
]
isn't 'n' == 'nparents'? Asking because right now I see O(n^4)
complexity and
I'd very much like to optimize it.
+
+ for (i = 0; i < n; i++) {
+ virJSONValuePtr obj = virJSONValueArrayGet(json_devicelist, i);
+ int nparents;
+
+ if (!virJSONValueIsObject(obj))
+ goto parsefailure;
+
+ nparents = virJSONValueObjectKeysNumber(obj);
+
+ for (j = 0; j < nparents; j++) {
+ const char *parent = virJSONValueObjectGetKey(obj, j);
+ virJSONValuePtr child_array = virJSONValueObjectGetValue(obj, j);
+ int nchildren;
+
+ if (!virJSONValueIsArray(child_array))
+ goto parsefailure;
+
+ nchildren = virJSONValueArraySize(child_array);
+
+ for (k = 0; k < nchildren; k++) {
+ virNodeDevCapMdevPtr mdev;
+ const char *uuid;
+ virJSONValuePtr props;
+ g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1);
+ virJSONValuePtr child_obj = virJSONValueArrayGet(child_array, k);
+
+ /* the child object should have a single key equal to its uuid.
+ * The value is an object describing the properties of the mdev */
+ if (virJSONValueObjectKeysNumber(child_obj) != 1)
+ goto parsefailure;
+
+ uuid = virJSONValueObjectGetKey(child_obj, 0);
+ props = virJSONValueObjectGetValue(child_obj, 0);
+
+ child->parent = g_strdup(parent);
+ child->caps = g_new0(virNodeDevCapsDef, 1);
+ child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;
+
+ mdev = &child->caps->data.mdev;
+ mdev->uuid = g_strdup(uuid);
+ mdev->type =
+ g_strdup(virJSONValueObjectGetString(props,
"mdev_type"));
+
+ virJSONValuePtr attrs = virJSONValueObjectGet(props,
"attrs");
+
+ if (attrs && virJSONValueIsArray(attrs)) {
+ int nattrs = virJSONValueArraySize(attrs);
+
+ mdev->attributes = g_new0(virMediatedDeviceAttrPtr, nattrs);
+ mdev->nattributes = nattrs;
+
+ for (m = 0; m < nattrs; m++) {
+ virJSONValuePtr attr = virJSONValueArrayGet(attrs, m);
+ virMediatedDeviceAttrPtr attribute;
+
+ if (!virJSONValueIsObject(attr) ||
+ virJSONValueObjectKeysNumber(attr) != 1)
+ goto parsefailure;
+
+ attribute = g_new0(virMediatedDeviceAttr, 1);
+ attribute->name = g_strdup(virJSONValueObjectGetKey(attr,
0));
+ virJSONValuePtr value = virJSONValueObjectGetValue(attr, 0);
+ attribute->value = g_strdup(virJSONValueGetString(value));
+ mdev->attributes[m] = attribute;
+ }
+ }
+ mdevGenerateDeviceName(child);
+
+ if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0)
+ child = NULL;
+ }
+ }
+ }
+
+ *devs = outdevs;
+ return noutdevs;
+
+ parsefailure:
+ for (i = 0; i < noutdevs; i++)
+ virNodeDeviceDefFree(outdevs[i]);
+ VIR_FREE(outdevs);
+
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("unable to parse JSON response"));
+ return -1;
+}
...
static void
nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv)
{
@@ -284,6 +366,15 @@ mymain(void)
#define DO_TEST_STOP(uuid) \
DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid)
+#define DO_TEST_LIST_DEFINED() \
+ do { \
+ if (virTestRun("mdevctl list --defined", testMdevctlListDefined, NULL)
< 0) \
How about we redefine the DO_TEST_FULL macro so that it doesn't pass a
reference to info on its own but forces the caller to do that? That way you
don't have to do ^this and simply pass NULL safely to DO_TEST_FULL.
Erik