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