[libvirt PATCH 5/6] nodedev: add mdev support to virNodeDeviceCreateXML()

[Erik Skultety]

On Thu, Apr 30, 2020 at 04:42:57PM -0500, Jonathon Jongsma wrote:
> With recent additions to the node device xml schema, an xml schema can
> now describe a mdev device sufficiently for libvirt to create and start
> the device using the mdevctl utility.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---

...

>  }
>  
> +static int
> +virNodeDeviceDefToMdevctlConfig(virNodeDeviceDefPtr def, char **buf)
> +{
> +    size_t i;
> +    virNodeDevCapMdevPtr mdev = &def->caps->data.mdev;
> +    g_autoptr(virJSONValue) json = virJSONValueNewObject();
> +
> +    if (virJSONValueObjectAppendString(json, "mdev_type", mdev->type) < 0)
> +        return -1;
> +    if (virJSONValueObjectAppendString(json, "start", "manual") < 0)
> +        return -1;
> +    if (mdev->attributes) {
> +        g_autoptr(virJSONValue) attributes = virJSONValueNewArray();
> +        for (i = 0; i < mdev->nattributes; i++) {
> +            virMediatedDeviceAttrPtr attr = mdev->attributes[i];
> +            g_autoptr(virJSONValue) jsonattr = virJSONValueNewObject();
> +
> +            if (virJSONValueObjectAppendString(jsonattr, attr->name, attr->value) < 0)
> +                return -1;
> +            if (virJSONValueArrayAppend(attributes, g_steal_pointer(&jsonattr)) < 0)
> +                return -1;
> +        }
> +        if (virJSONValueObjectAppend(json, "attrs", g_steal_pointer(&attributes)) < 0)
> +            return -1;
> +    }
> +
> +    *buf = virJSONValueToString(json, false);
> +    if (*buf == NULL)

tiny nitpick - we test pointers simply as if(!ptr)   (multiple occurrences)

> +        return -1;
> +
> +    return 0;
> +}
> +
> +static int
> +virMdevctlStart(const char *parent, const char *json_file, char **uuid)
> +{
> +    int status;
> +    g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);

insert an empty line here...

> +    if (!mdevctl)
> +        return -1;
> +
> +    g_autoptr(virCommand) cmd = virCommandNewArgList(mdevctl,
> +                                                     "start",
> +                                                     "-p",
> +                                                     parent,
> +                                                     "--jsonfile",
> +                                                     json_file,
> +                                                     NULL);

So, I was wondering what if someone actually wants to specify the UUID for the
device for some reason. We would have to expose a new element in the XML
partially duplicating what the <name> already contains. On the other hand,
I think we're good in create since these are truly supposed to be transient
mdevs, so it's desirable to let someone else (or us internally for that matter)
figure out the UUID for such device.

Now, this is irrelevant to this patch because I'm already thinking ahead.
In order to define an mdev, you will need to expose an element mapping to the
UUID, you want to let apps create mdev profiles with expected UUID values and
activate them on demand.


> +    virCommandAddEnvPassCommon(cmd);
> +    virCommandSetOutputBuffer(cmd, uuid);
> +
> +    /* an auto-generated uuid is returned via stdout if no uuid is specified in
> +     * the mdevctl args */
> +    if (virCommandRun(cmd, &status) < 0 || status != 0)
> +        return -1;
> +
> +    /* remove newline */
> +    *uuid = g_strstrip(*uuid);
> +
> +    return 0;
> +}
> +
>  virNodeDevicePtr
>  nodeDeviceCreateXML(virConnectPtr conn,
>                      const char *xmlDesc,
> @@ -569,6 +674,78 @@ nodeDeviceCreateXML(virConnectPtr conn,
>                             _("no node device for '%s' with matching "
>                               "wwnn '%s' and wwpn '%s'"),
>                             def->name, wwnn, wwpn);
> +    } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
> +        virNodeDeviceObjPtr parent_dev = NULL;
> +        virNodeDeviceDefPtr parent_def = NULL;
> +        virNodeDevCapsDefPtr parent_caps = NULL;
> +        g_autofree char *uuid = NULL;
> +        g_autofree char *parent_pci = NULL;
> +
> +        if (def->parent == NULL) {
> +            virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",

This IMO should be a VIR_ERR_XML_ERROR because it's a mandatory element to be
specified.

> +                           _("cannot create a mediated device without a parent"));
> +            return NULL;
> +        }
> +
> +        if ((parent_dev = virNodeDeviceObjListFindByName(driver->devs, def->parent)) == NULL) {
> +            virReportError(VIR_ERR_NO_NODE_DEVICE,
> +                           _("could not find parent device '%s'"), def->parent);
> +            return NULL;
> +        }
> +
> +        parent_def = virNodeDeviceObjGetDef(parent_dev);
> +        for (parent_caps = parent_def->caps; parent_caps != NULL; parent_caps = parent_caps->next) {
> +            if (parent_caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
> +                virPCIDeviceAddress addr = {
> +                    .domain = parent_caps->data.pci_dev.domain,
> +                    .bus = parent_caps->data.pci_dev.bus,
> +                    .slot = parent_caps->data.pci_dev.slot,
> +                    .function = parent_caps->data.pci_dev.function
> +                };
> +
> +                parent_pci = virPCIDeviceAddressAsString(&addr);
> +                break;
> +            }
> +        }
> +
> +        virNodeDeviceObjEndAPI(&parent_dev);
> +
> +        if (parent_pci == NULL) {
> +            virReportError(VIR_ERR_NO_NODE_DEVICE,
> +                           _("unable to find PCI address for parent device '%s'"), def->parent);
> +            return NULL;
> +        }
> +
> +        /* Convert node device definition to a JSON string formatted for
> +         * mdevctl */
> +        g_autofree char *json = NULL;
> +        if (virNodeDeviceDefToMdevctlConfig(def, &json) < 0) {
> +            virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",

I think ^this should be an INTERNAL_ERROR instead

> +                           _("couldn't convert node device def to mdevctl JSON"));
> +            return NULL;
> +        }
> +
> +        g_autofree char *tmp = g_build_filename(driver->stateDir, "mdev-json-XXXXXX", NULL);
> +        gint tmpfd;
> +
> +        if ((tmpfd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) {
> +            virReportSystemError(errno, "%s", _("Failed to open temp file for write"));
> +            return NULL;
> +        }
> +        if (safewrite(tmpfd, json, strlen(json)) != strlen(json) ||
> +            VIR_CLOSE(tmpfd) < 0) {
> +            virReportSystemError(errno, "%s", _("Failed to write temp file"));
> +            return NULL;

You should discard the temporary JSON file at some point once the below is
finished because it's littering the state dir.

> +        }
> +
> +        if (virMdevctlStart(parent_pci, tmp, &uuid) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Unable to start mediated device"));
> +            return NULL;
> +        }
> +
> +        if (uuid && uuid[0] != '\0')

Is ^this check necessary? I mean, mdevctl either returns the UUID and retcode 0
or nothing and retcode 1 which we should have already checked in virCommandRun,
so since it was successful, we now know that UUID is filled in or did I miss
something in the code?

> +            device = nodeDeviceFindNewMediatedDevice(conn, uuid);
>      } else {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("Unsupported device type"));

-- 
Erik Skultety