[libvirt PATCH 5/6] nodedev: add mdev support to virNodeDeviceCreateXML()
Michal Privoznik
mprivozn at redhat.com
Mon May 11 13:27:56 UTC 2020
On 4/30/20 11:42 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.
>
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
> libvirt.spec.in | 3 +
> 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 | 177 +++++++++++++++++++++++++++
> 6 files changed, 221 insertions(+)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 6abf97df85..411846a9fc 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -466,6 +466,9 @@ Requires: dbus
> # For uid creation during pre
> Requires(pre): shadow-utils
>
> +# For managing persistent mediated devices
> +Recommends: mdevctl
> +
I wonder whether we could make this for nodedev driver RPM only.
Because, if somebody installs say client library + virsh only (for
remote mgmt), then there is no reason they should install mdevctl with that.
> %description daemon
> Server side daemon required to manage the virtualization capabilities
> of recent versions of Linux. Requires a hypervisor specific sub-RPM
> 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])
>
> 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 94b206aa21..0468f68f52 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 f948dfbf69..c271f3bae5 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,
> @@ -488,6 +514,23 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
> return device;
> }
>
> +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 _NewSCISHostFuncData
> {
> const char *wwnn;
> @@ -525,6 +568,68 @@ nodeDeviceHasCapability(virNodeDeviceDefPtr def, virNodeDevCapType type)
> return false;
> }
>
> +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)
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> +virMdevctlStart(const char *parent, const char *json_file, char **uuid)
> +{
> + int status;
> + g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);
> + if (!mdevctl)
> + return -1;
> +
> + g_autoptr(virCommand) cmd = virCommandNewArgList(mdevctl,
> + "start",
> + "-p",
> + parent,
> + "--jsonfile",
> + json_file,
> + NULL);
> + 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)) {
Can this whole block be moved into a separate function? I think it would
look slightly better.
> + 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",
> + _("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",
> + _("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;
These variables should be declared upfront. We don't really like
in-block declarations. Also, we have VIR_AUTOCLOSE ;-) Or even better,
virFileWriteStr(). I know, it's not obvious, unless you know src/util/
by heart.
> +
> + 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;
> + }
> +
> + 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')
> + device = nodeDeviceFindNewMediatedDevice(conn, uuid);
> } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Unsupported device type"));
>
Michal
More information about the libvir-list
mailing list