[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt PATCH v2 05/16] nodedev: add ability to list and parse defined mdevs



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]