On Tue, Jun 16, 2020 at 09:27:54AM -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.
>
> Note that some of the the configuration for a mediated device must be
> passed to mdevctl as a JSON-formatted file. In order to avoid creating
> and cleaning up temporary files, the JSON is instead fed to stdin and we
> pass the filename /dev/stdin to mdevctl. While this may not be portable,
> neither are mediated devices, so I don't believe it should cause any
> problems.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
...
> +
>
> char *
> nodeDeviceGetXMLDesc(virNodeDevicePtr device,
> @@ -492,6 +518,25 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
> }
>
>
> +static virNodeDevicePtr
> +nodeDeviceFindNewMediatedDeviceFunc(virConnectPtr conn,
> + const void *opaque)
> +{
> + const char *uuid = opaque;
empty line here..
> + return nodeDeviceLookupMediatedDeviceByUUID(conn, uuid, 0);
> +}
> +
> +
> +static virNodeDevicePtr
> +nodeDeviceFindNewMediatedDevice(virConnectPtr conn,
> + const char *mdev_uuid)
> +{
> + return nodeDeviceFindNewDevice(conn,
> + nodeDeviceFindNewMediatedDeviceFunc,
> + mdev_uuid);
> +}
> +
> +
> typedef struct _NewSCSIHostFuncData NewSCSIHostFuncData;
> struct _NewSCSIHostFuncData
> {
> @@ -534,6 +579,158 @@ nodeDeviceHasCapability(virNodeDeviceDefPtr def, virNodeDevCapType type)
> }
>
>
> +/* format a json string that provides configuration information about this mdev
> + * to the mdevctl utility */
> +static int
> +nodeDeviceDefToMdevctlConfig(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;
here...
> + if (virJSONValueObjectAppendString(json, "start", "manual") < 0)
> + return -1;
here...
> + if (mdev->attributes) {
> + g_autoptr(virJSONValue) attributes = virJSONValueNewArray();
here...
> + 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;
here...
> + if (virJSONValueArrayAppend(attributes, g_steal_pointer(&jsonattr)) < 0)
> + return -1;
> + }
and here...
> + if (virJSONValueObjectAppend(json, "attrs", g_steal_pointer(&attributes)) < 0)
> + return -1;
> + }
> +
> + *buf = virJSONValueToString(json, false);
> + if (!*buf)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +static char *
> +nodeDeviceFindAddressByName(const char *name)
> +{
> + virNodeDeviceDefPtr def = NULL;
> + virNodeDevCapsDefPtr caps = NULL;
> + char *pci_addr = NULL;
> + virNodeDeviceObjPtr dev = virNodeDeviceObjListFindByName(driver->devs, name);
> +
> + if (!dev) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE,
> + _("could not find device '%s'"), name);
> + return NULL;
> + }
> +
> + def = virNodeDeviceObjGetDef(dev);
> + for (caps = def->caps; caps != NULL; caps = caps->next) {
> + if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
> + virPCIDeviceAddress addr = {
> + .domain = caps->data.pci_dev.domain,
> + .bus = caps->data.pci_dev.bus,
> + .slot = caps->data.pci_dev.slot,
> + .function = caps->data.pci_dev.function
> + };
> +
> + pci_addr = virPCIDeviceAddressAsString(&addr);
> + break;
> + }
> + }
> +
> + virNodeDeviceObjEndAPI(&dev);
> +
> + return pci_addr;
> +}
> +
> +
> +virCommandPtr
> +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> + bool persist,
> + char **uuid_out)
> +{
> + virCommandPtr cmd;
> + const char *subcommand;
> + g_autofree char *json = NULL;
> + g_autofree char *parent_pci = nodeDeviceFindAddressByName(def->parent);
> +
> + if (!parent_pci) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE,
> + _("unable to find PCI address for parent device '%s'"), def->parent);
> + return NULL;
> + }
> +
> + if (nodeDeviceDefToMdevctlConfig(def, &json) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("couldn't convert node device def to mdevctl JSON"));
> + return NULL;
> + }
> +
> + if (persist)
> + subcommand = "define";
Like I mentioned in v2 of this patch, "define" doesn't start anything without
--auto added to the cmdline. Still, it feels like the function is doing
a tiny bit more than it should IMO. You also noted that you're preparing for when we
start defining mdevs, I'd say let's leave 'persist' out until we actually
introduce support for persisting mdevs.
The rest of the patch looks good to me.
Regards,
Erik
