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