[libvirt PATCH v2 05/10] nodedev: add mdev support to virNodeDeviceCreateXML()

Michal Privoznik mprivozn at redhat.com
Wed Jun 10 18:01:45 UTC 2020


On 6/9/20 11:43 PM, 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.
> 

Agreed. mdevs are Linux specific and /dev/stdin is UNIX-wide understood 
(which is a strict superset of the former), so we are safe on that front.

> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>   libvirt.spec.in                      |   2 +
>   m4/virt-external-programs.m4         |   3 +
>   src/conf/virnodedeviceobj.c          |  34 +++++
>   src/conf/virnodedeviceobj.h          |   3 +
>   src/libvirt_private.syms             |   1 +
>   src/node_device/node_device_driver.c | 203 +++++++++++++++++++++++++++
>   src/node_device/node_device_driver.h |   6 +
>   7 files changed, 252 insertions(+)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 262e66f3cc..2b3a4d2e71 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -522,6 +522,8 @@ Requires: libvirt-daemon = %{version}-%{release}
>   Requires: libvirt-libs = %{version}-%{release}
>   # needed for device enumeration
>   Requires: systemd >= 185
> +# For managing persistent mediated devices
> +Requires: mdevctl
>   
>   %description daemon-driver-nodedev
>   The nodedev driver plugin for the libvirtd daemon, providing
> diff --git a/m4/virt-external-programs.m4 b/m4/virt-external-programs.m4
> index 9046e3bf07..bd3cb1f757 100644
> --- a/m4/virt-external-programs.m4
> +++ b/m4/virt-external-programs.m4
> @@ -65,6 +65,7 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [
>     AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], [$LIBVIRT_SBIN_PATH])
>     AC_PATH_PROG([SCRUB], [scrub], [scrub], [$LIBVIRT_SBIN_PATH])
>     AC_PATH_PROG([ADDR2LINE], [addr2line], [addr2line], [$LIBVIRT_SBIN_PATH])
> +  AC_PATH_PROG([MDEVCTL], [mdevctl], [mdevctl], [$LIBVIRT_SBIN_PATH])

This will need to look slightly different - see 07/10.

>   
>     AC_DEFINE_UNQUOTED([DMIDECODE], ["$DMIDECODE"],
>                        [Location or name of the dmidecode program])
> @@ -88,6 +89,8 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [
>                        [Location or name of the scrub program (for wiping algorithms)])
>     AC_DEFINE_UNQUOTED([ADDR2LINE], ["$ADDR2LINE"],
>                        [Location of addr2line program])
> +  AC_DEFINE_UNQUOTED([MDEVCTL], ["$MDEVCTL"],
> +                     [Location or name of the mdevctl program])
>   
>     AC_PATH_PROG([IP_PATH], [ip], [/sbin/ip], [$LIBVIRT_SBIN_PATH])
>     AC_DEFINE_UNQUOTED([IP_PATH], ["$IP_PATH"], [path to ip binary])
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index 3a34a324ca..fd20d5f9e2 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -399,6 +399,40 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
>                                         &data);
>   }
>   
> +static int
> +virNodeDeviceObjListFindMediatedDeviceByUUIDCallback(const void *payload,
> +                                                     const void *name G_GNUC_UNUSED,
> +                                                     const void *opaque G_GNUC_UNUSED)
> +{
> +    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> +    const char *uuid = (const char *) opaque;
> +    virNodeDevCapsDefPtr cap;
> +    int want = 0;
> +
> +    virObjectLock(obj);
> +
> +    for (cap = obj->def->caps; cap != NULL; cap = cap->next) {
> +        if (cap->data.type == VIR_NODE_DEV_CAP_MDEV) {
> +            if (STREQ(cap->data.mdev.uuid, uuid)) {
> +                want = 1;
> +                break;
> +            }
> +        }
> +     }
> +
> +    virObjectUnlock(obj);
> +    return want;
> +}
> +
> +
> +virNodeDeviceObjPtr
> +virNodeDeviceObjListFindMediatedDeviceByUUID(virNodeDeviceObjListPtr devs,
> +                                             const char *uuid)
> +{
> +    return virNodeDeviceObjListSearch(devs,
> +                                      virNodeDeviceObjListFindMediatedDeviceByUUIDCallback,
> +                                      uuid);
> +}
>   
>   static void
>   virNodeDeviceObjListDispose(void *obj)
> diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
> index c9df8dedab..6efdb23d36 100644
> --- a/src/conf/virnodedeviceobj.h
> +++ b/src/conf/virnodedeviceobj.h
> @@ -118,3 +118,6 @@ virNodeDeviceObjListExport(virConnectPtr conn,
>   void
>   virNodeDeviceObjSetSkipUpdateCaps(virNodeDeviceObjPtr obj,
>                                     bool skipUpdateCaps);
> +virNodeDeviceObjPtr
> +virNodeDeviceObjListFindMediatedDeviceByUUID(virNodeDeviceObjListPtr devs,
> +                                             const char *uuid);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b8e6f058c3..532169d93d 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1178,6 +1178,7 @@ virNodeDeviceObjListAssignDef;
>   virNodeDeviceObjListExport;
>   virNodeDeviceObjListFindByName;
>   virNodeDeviceObjListFindBySysfsPath;
> +virNodeDeviceObjListFindMediatedDeviceByUUID;
>   virNodeDeviceObjListFindSCSIHostByWWNs;
>   virNodeDeviceObjListFree;
>   virNodeDeviceObjListGetNames;
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 629d4bcf91..dbc7eb4d1e 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -30,6 +30,7 @@
>   #include "datatypes.h"
>   #include "viralloc.h"
>   #include "virfile.h"
> +#include "virjson.h"
>   #include "virstring.h"
>   #include "node_device_conf.h"
>   #include "node_device_event.h"
> @@ -40,6 +41,7 @@
>   #include "viraccessapicheck.h"
>   #include "virnetdev.h"
>   #include "virutil.h"
> +#include "vircommand.h"
>   
>   #define VIR_FROM_THIS VIR_FROM_NODEDEV
>   
> @@ -304,6 +306,30 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>       return device;
>   }
>   
> +static virNodeDevicePtr
> +nodeDeviceLookupMediatedDeviceByUUID(virConnectPtr conn,
> +                                     const char *uuid,
> +                                     unsigned int flags)
> +{
> +    virNodeDeviceObjPtr obj = NULL;
> +    virNodeDeviceDefPtr def;
> +    virNodeDevicePtr device = NULL;
> +
> +    virCheckFlags(0, NULL);
> +
> +    if (!(obj = virNodeDeviceObjListFindMediatedDeviceByUUID(driver->devs,
> +                                                             uuid)))
> +        return NULL;
> +
> +    def = virNodeDeviceObjGetDef(obj);
> +
> +    if ((device = virGetNodeDevice(conn, def->name)))
> +        device->parentName = g_strdup(def->parent);
> +
> +    virNodeDeviceObjEndAPI(&obj);
> +    return device;
> +}
> +
>   
>   char *
>   nodeDeviceGetXMLDesc(virNodeDevicePtr device,
> @@ -492,6 +518,25 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
>   }
>   
>   
> +static virNodeDevicePtr
> +nodeDeviceFindNewMediatedDeviceFunc(virConnectPtr conn,
> +                                    const void *opaque)
> +{
> +    const char *uuid = opaque;
> +    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,162 @@ 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;
> +    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)
> +        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 *mdevctl = virFindFileInPath(MDEVCTL);
> +    g_autofree char *parent_pci = nodeDeviceFindAddressByName(def->parent);
> +
> +    if (!mdevctl)
> +        return NULL;

This virFindFileInPath() and @mdevctl variable aren't necessary. 
virCommandRun() will call virFindFileInPath() under the hood (see 
v6.4.0-66-gf603b99ad9). And also, if the function fails, it does so 
silently. And this doesn't report error neither.

> +
> +    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";
> +    else
> +        subcommand = "start";
> +
> +    cmd = virCommandNewArgList(mdevctl, subcommand,
> +                               "-p", parent_pci,
> +                               "--jsonfile", "/dev/stdin",
> +                               NULL);
> +
> +    virCommandSetInputBuffer(cmd, json);
> +    virCommandSetOutputBuffer(cmd, uuid_out);
> +
> +    return cmd;
> +}
> +
> +static int
> +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
> +{
> +    int status;
> +    g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, false,
> +                                                                 uuid);
> +    if (!cmd)
> +        return -1;
> +
> +    /* 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;
> +}
> +
> +
> +static virNodeDevicePtr
> +nodeDeviceCreateXMLMdev(virConnectPtr conn,
> +                        virNodeDeviceDefPtr def)
> +{
> +    g_autofree char *uuid = NULL;
> +
> +    if (!def->parent) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("cannot create a mediated device without a parent"));
> +        return NULL;
> +    }
> +
> +    if (virMdevctlStart(def, &uuid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Unable to start mediated device"));
> +        return NULL;
> +    }
> +
> +    return nodeDeviceFindNewMediatedDevice(conn, uuid);
> +}
> +
> +
>   virNodeDevicePtr
>   nodeDeviceCreateXML(virConnectPtr conn,
>                       const char *xmlDesc,
> @@ -578,6 +779,8 @@ 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)) {
> +        device = nodeDeviceCreateXMLMdev(conn, def);
>       } else {
>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                          _("Unsupported device type"));
> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
> index eae5e2cb17..576f75375f 100644
> --- a/src/node_device/node_device_driver.h
> +++ b/src/node_device/node_device_driver.h
> @@ -24,6 +24,7 @@
>   #include "internal.h"
>   #include "driver.h"
>   #include "virnodedeviceobj.h"
> +#include "vircommand.h"
>   
>   #define LINUX_NEW_DEVICE_WAIT_TIME 60
>   
> @@ -116,3 +117,8 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn,
>   int
>   nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
>                                           int callbackID);
> +
> +virCommandPtr
> +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> +                                 bool persist,
> +                                 char **uuid_out);
> 




More information about the libvir-list mailing list