On 03/22/2017 11:27 AM, Erik Skultety wrote: > Beside creation, disposal, getter, and setter methods the module exports > methods to work with lists of mediated devices. > > Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx> > --- > po/POTFILES.in | 1 + > src/Makefile.am | 1 + > src/libvirt_private.syms | 22 +++ > src/util/virmdev.c | 487 +++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virmdev.h | 123 ++++++++++++ > 5 files changed, 634 insertions(+) > create mode 100644 src/util/virmdev.c > create mode 100644 src/util/virmdev.h > > diff --git a/po/POTFILES.in b/po/POTFILES.in > index 64cb88cfa5..df3d3cfe80 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -224,6 +224,7 @@ src/util/virlease.c > src/util/virlockspace.c > src/util/virlog.c > src/util/virmacmap.c > +src/util/virmdev.c > src/util/virnetdev.c > src/util/virnetdevbandwidth.c > src/util/virnetdevbridge.c > diff --git a/src/Makefile.am b/src/Makefile.am > index 3b1bb1da35..01b7f4e0be 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -188,6 +188,7 @@ UTIL_SOURCES = \ > util/virvhba.c util/virvhba.h \ > util/virxdrdefs.h \ > util/virxml.c util/virxml.h \ > + util/virmdev.c util/virmdev.h \ > $(NULL) > > EXTRA_DIST += $(srcdir)/util/keymaps.csv $(srcdir)/util/virkeycode-mapgen.py > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 57acfdbb19..c51b295d30 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1983,6 +1983,28 @@ virMacMapNew; > virMacMapRemove; > virMacMapWriteFile; > > +# util/virmdev.h > +virMediatedDeviceFree; > +virMediatedDeviceGetIOMMUGroupDev; > +virMediatedDeviceGetIOMMUGroupNum; > +virMediatedDeviceGetSysfsPath; > +virMediatedDeviceGetUsedBy; > +virMediatedDeviceIsUsed; > +virMediatedDeviceListAdd; > +virMediatedDeviceListCount; > +virMediatedDeviceListDel; > +virMediatedDeviceListFind; > +virMediatedDeviceListGet; > +virMediatedDeviceListMarkDevices; > +virMediatedDeviceListNew; > +virMediatedDeviceListSteal; > +virMediatedDeviceListStealIndex; > +virMediatedDeviceModelTypeFromString; > +virMediatedDeviceModelTypeToString; > +virMediatedDeviceNew; > +virMediatedDeviceSetUsedBy; > + > + > > # util/virnetdev.h > virNetDevAddMulti; > diff --git a/src/util/virmdev.c b/src/util/virmdev.c > new file mode 100644 > index 0000000000..d1692a982a > --- /dev/null > +++ b/src/util/virmdev.c > @@ -0,0 +1,487 @@ > +/* > + * virmdev.c: helper APIs for managing host mediated devices > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>. > + */ > + > +#include <config.h> > + > +#include <fcntl.h> > +#include <inttypes.h> > +#include <limits.h> > +#include <stdio.h> > +#include <string.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <unistd.h> > +#include <stdlib.h> None of the above are needed. Compilation completes just fine without any of them. > + > +#include "virmdev.h" > +#include "dirname.h" dirname.h is from gnulib. Move it out separate from all the vir*.h includes. > +#include "virlog.h" > +#include "viralloc.h" > +#include "vircommand.h" > +#include "virerror.h" > +#include "virfile.h" > +#include "virkmod.h" > +#include "virstring.h" > +#include "virutil.h" > +#include "viruuid.h" Out of all the above, I found that nothing is used from vircommand.h, virkmod.h, or viruuid.h, so they definitely should be removed. In addition, it looks like maybe nothing from virutil.h is used either (compiles fine without it). It also compiles okay without virerror.h, but that's just because it's incidentally included by some other include, so it can/should stay. > + > +#define VIR_FROM_THIS VIR_FROM_NONE > + > +VIR_LOG_INIT("util.mdev"); > + > +struct _virMediatedDevice { > + char *path; /* sysfs path */ > + virMediatedDeviceModelType model; > + > + char *used_by_drvname; > + char *used_by_domname; > +}; > + > +struct _virMediatedDeviceList { > + virObjectLockable parent; > + > + size_t count; > + virMediatedDevicePtr *devs; > +}; > + > +VIR_ENUM_IMPL(virMediatedDeviceModel, VIR_MDEV_MODEL_TYPE_LAST, > + "vfio-pci") > + > +static virClassPtr virMediatedDeviceListClass; > + > +static void virMediatedDeviceListDispose(void *obj); > + > +static int virMediatedOnceInit(void) > +{ > + if (!(virMediatedDeviceListClass = virClassNew(virClassForObjectLockable(), > + "virMediatedDeviceList", > + sizeof(virMediatedDeviceList), > + virMediatedDeviceListDispose))) > + return -1; > + > + return 0; > +} > + > +VIR_ONCE_GLOBAL_INIT(virMediated) (I really wish there was a less verbose way to do all the above. Not your problem though...) > + > +static int > +virMediatedDeviceGetSysfsDeviceAPI(virMediatedDevicePtr dev, > + char **device_api) > +{ > + int ret = -1; > + char *buf = NULL; > + char *tmp = NULL; > + char *file = NULL; Since buf and file both point to strings that we need to free, and tmp doesn't, I think it would be better if tmp was after the others and not initialized since it's never used before it's set) to differentiate it from the other two (you can't differentiate by making it const, since it's used to replace the \n with 0. A comment saying that anything it points to doesn't need to be freed might be nice too. (Generally I assume that const char* doesn't need its data freed, but char* does. Seeing it not freed sets off alarms that then need brain cells to suppress). > + > + if (virAsprintf(&file, "%s/mdev_type/device_api", dev->path) < 0) > + goto cleanup; > + > + /* TODO - make this a generic method to access sysfs files for various > + * kinds of devices > + */ > + if (!virFileExists(file)) { > + virReportSystemError(errno, _("Failed to read '%s'"), file); We're consistently inconsistent about it throughout the code (and I am personally inconsisent about it even within the same path) but I *think* there is a preference for starting error messages with lower case. Not worth an edit just for that though. > + goto cleanup; > + } > + > + if (virFileReadAll(file, 1024, &buf) < 0) > + goto cleanup; If it was 1983, or even 1990, I would complain that you're going to use up 1024 bytes for a string that will surely never be longer than 15-20 characters. But of course we no longer run our software on Z-80s with 64k of RAM, so I must keep my mouth shut :-) (I haven't looked in many years, but I would be surprised if a modern malloc gave out memory in chunks less than 1024 bytes anyway...) > + > + if ((tmp = strchr(buf, '\n'))) > + *tmp = '\0'; > + > + *device_api = buf; > + buf = NULL; > + > + ret = 0; > + cleanup: > + VIR_FREE(file); > + VIR_FREE(buf); > + return ret; > +} > + > + > +static int > +virMediatedDeviceCheckModel(virMediatedDevicePtr dev, > + virMediatedDeviceModelType model) > +{ > + int ret = -1; > + char *dev_api = NULL; > + int actual_model; > + > + if (virMediatedDeviceGetSysfsDeviceAPI(dev, &dev_api) < 0) > + return -1; > + > + /* safeguard in case we've got an older libvirt which doesn't know newer > + * device_api models yet > + */ > + if ((actual_model = virMediatedDeviceModelTypeFromString(dev_api)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Device API '%s' not supported yet"), > + dev_api); > + goto cleanup; > + } > + > + if (actual_model != model) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Invalid device API '%s' for device %s: " > + "device only supports '%s'"), > + virMediatedDeviceModelTypeToString(model), > + dev->path, dev_api); > + goto cleanup; > + } > + > + ret = 0; > + cleanup: > + VIR_FREE(dev_api); > + return ret; > +} > + > + > +#ifdef __linux__ Why did you only start the #ifdef part here - surely most of the stuff up above would never be used on a non-linux system either? virMediatedDeviceGetSysfsDeviceAPI for example references files in linux sysfs. > +# define MDEV_SYSFS_DEVICES "/sys/bus/mdev/devices/" > + > +virMediatedDevicePtr > +virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) > +{ > + virMediatedDevicePtr ret = NULL; > + virMediatedDevicePtr dev = NULL; > + > + if (VIR_ALLOC(dev) < 0) > + return NULL; > + > + if (!(dev->path = virMediatedDeviceGetSysfsPath(uuidstr))) > + goto cleanup; > + > + /* Check whether the user-provided model corresponds with the actually > + * supported mediated device's API. > + */ > + if (virMediatedDeviceCheckModel(dev, model)) > + goto cleanup; > + > + dev->model = model; > + VIR_STEAL_PTR(ret, dev); Heh. Never noticed that one before :-) > + > + cleanup: > + virMediatedDeviceFree(dev); > + return ret; > +} > + > +#else > + > +virMediatedDevicePtr > +virMediatedDeviceNew(virPCIDeviceAddressPtr pciaddr ATTRIBUTE_UNUSED, > + const char *uuidstr ATTRIBUTE_UNUSED) > +{ > + virRerportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("not supported on non-linux platforms")); Maybe say "mediate devices ...." in case someone sees/reports the log message from the output of virsh (where the function name isn't a part of the log message). > + return NULL; > +} > + > +#endif /* __linux__ */ > + > +void > +virMediatedDeviceFree(virMediatedDevicePtr dev) > +{ > + if (!dev) > + return; > + VIR_FREE(dev->path); > + VIR_FREE(dev->used_by_drvname); > + VIR_FREE(dev->used_by_domname); > + VIR_FREE(dev); > +} > + > + > +const char * > +virMediatedDeviceGetPath(virMediatedDevicePtr dev) > +{ > + return dev->path; > +} > + > + > +/* Returns an absolute canonicalized path to the device used to control the > + * mediated device's IOMMU group (e.g. "/dev/vfio/15"). Caller is responsible > + * for freeing the result. > + */ > +char * > +virMediatedDeviceGetIOMMUGroupDev(virMediatedDevicePtr dev) > +{ > + char *resultpath = NULL; > + char *iommu_path = NULL; > + char *vfio_path = NULL; > + > + if (virAsprintf(&iommu_path, "%s/iommu_group", dev->path) < 0) > + return NULL; > + > + if (!virFileExists(iommu_path)) { > + virReportSystemError(errno, _("Failed to access '%s'"), iommu_path); > + goto cleanup; > + } > + > + if (virFileResolveLink(iommu_path, &resultpath) < 0) { > + virReportSystemError(errno, _("Failed to resolve '%s'"), iommu_path); > + goto cleanup; > + } > + > + if (virAsprintf(&vfio_path, "/dev/vfio/%s", last_component(resultpath)) < 0) > + goto cleanup; > + > + cleanup: > + VIR_FREE(resultpath); > + VIR_FREE(iommu_path); > + return vfio_path; > +} > + > + > +int > +virMediatedDeviceGetIOMMUGroupNum(virMediatedDevicePtr dev) > +{ > + char *vfio_path = NULL; > + char *group_num_str = NULL; > + unsigned int group_num = -1; > + > + if (!(vfio_path = virMediatedDeviceGetIOMMUGroupDev(dev))) > + return -1; > + > + group_num_str = last_component(vfio_path); > + ignore_value(virStrToLong_ui(group_num_str, NULL, 10, &group_num)); I'm assuming the caller logs a message on failure? If so, then the ignore_value is okay. (end of comments) To sum it up: ACK if you: * remove the unnecessary #includes * make it obvious when defined that tmp doesn't "own" any memory that should be freed (just to make life easier for maintainers) * modify the "unsupported" log message to start with "mediated devices" * maybe put more/most/all(?) of the functions inside #ifdef __linux__ > + > + VIR_FREE(vfio_path); > + return group_num; > +} > + > + > +void > +virMediatedDeviceGetUsedBy(virMediatedDevicePtr dev, > + const char **drvname, const char **domname) > +{ > + *drvname = dev->used_by_drvname; > + *domname = dev->used_by_domname; > +} > + > + > +int > +virMediatedDeviceSetUsedBy(virMediatedDevicePtr dev, > + const char *drvname, > + const char *domname) > +{ > + VIR_FREE(dev->used_by_drvname); > + VIR_FREE(dev->used_by_domname); > + if (VIR_STRDUP(dev->used_by_drvname, drvname) < 0) > + return -1; > + if (VIR_STRDUP(dev->used_by_domname, domname) < 0) > + return -1; > + > + return 0; > +} > + > + > +virMediatedDeviceListPtr > +virMediatedDeviceListNew(void) > +{ > + virMediatedDeviceListPtr list; > + > + if (virMediatedInitialize() < 0) > + return NULL; > + > + if (!(list = virObjectLockableNew(virMediatedDeviceListClass))) > + return NULL; > + > + return list; > +} > + > + > +static void > +virMediatedDeviceListDispose(void *obj) > +{ > + virMediatedDeviceListPtr list = obj; > + size_t i; > + > + for (i = 0; i < list->count; i++) { > + virMediatedDeviceFree(list->devs[i]); > + list->devs[i] = NULL; > + } > + > + list->count = 0; > + VIR_FREE(list->devs); > +} > + > + > +int > +virMediatedDeviceListAdd(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev) > +{ > + if (virMediatedDeviceListFind(list, dev)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Device %s is already in use"), dev->path); > + return -1; > + } > + return VIR_APPEND_ELEMENT(list->devs, list->count, dev); > +} > + > + > +virMediatedDevicePtr > +virMediatedDeviceListGet(virMediatedDeviceListPtr list, > + ssize_t idx) > +{ > + if (idx < 0 || idx >= list->count) > + return NULL; > + > + return list->devs[idx]; > +} > + > + > +size_t > +virMediatedDeviceListCount(virMediatedDeviceListPtr list) > +{ > + return list->count; > +} > + > + > +virMediatedDevicePtr > +virMediatedDeviceListStealIndex(virMediatedDeviceListPtr list, > + ssize_t idx) > +{ > + virMediatedDevicePtr ret; > + > + if (idx < 0 || idx >= list->count) > + return NULL; > + > + ret = list->devs[idx]; > + VIR_DELETE_ELEMENT(list->devs, idx, list->count); > + return ret; > +} > + > + > +virMediatedDevicePtr > +virMediatedDeviceListSteal(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev) > +{ > + int idx = virMediatedDeviceListFindIndex(list, dev); > + > + return virMediatedDeviceListStealIndex(list, idx); > +} > + > + > +void > +virMediatedDeviceListDel(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev) > +{ > + virMediatedDevicePtr ret = virMediatedDeviceListSteal(list, dev); > + virMediatedDeviceFree(ret); > +} > + > + > +int > +virMediatedDeviceListFindIndex(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev) > +{ > + size_t i; > + > + for (i = 0; i < list->count; i++) { > + virMediatedDevicePtr other = list->devs[i]; > + if (STREQ(other->path, dev->path)) > + return i; > + } > + return -1; > +} > + > + > +virMediatedDevicePtr > +virMediatedDeviceListFind(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev) > +{ > + int idx; > + > + if ((idx = virMediatedDeviceListFindIndex(list, dev)) >= 0) > + return list->devs[idx]; > + else > + return NULL; > +} > + > + > +bool > +virMediatedDeviceIsUsed(virMediatedDevicePtr dev, > + virMediatedDeviceListPtr list) > +{ > + const char *drvname, *domname; > + virMediatedDevicePtr tmp = NULL; > + > + if ((tmp = virMediatedDeviceListFind(list, dev))) { > + virMediatedDeviceGetUsedBy(tmp, &drvname, &domname); > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("Mediated device %s is in use by " > + "driver %s, domain %s"), > + tmp->path, drvname, domname); > + } > + > + return !!tmp; > +} > + > + > +char * > +virMediatedDeviceGetSysfsPath(const char *uuidstr) > +{ > + char *ret = NULL; > + > + ignore_value(virAsprintf(&ret, MDEV_SYSFS_DEVICES "%s", uuidstr)); > + return ret; > +} > + > + > +int > +virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst, > + virMediatedDeviceListPtr src, > + const char *drvname, > + const char *domname) > +{ > + int ret = -1; > + size_t count = virMediatedDeviceListCount(src); > + size_t i, j; > + > + virObjectLock(dst); > + for (i = 0; i < count; i++) { > + virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i); > + > + if (virMediatedDeviceIsUsed(mdev, dst) || > + virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0) > + goto cleanup; > + > + /* Copy mdev references to the driver list: > + * - caller is responsible for NOT freeing devices in @list on success > + * - we're responsible for performing a rollback on failure > + */ > + if (virMediatedDeviceListAdd(dst, mdev) < 0) > + goto rollback; > + > + VIR_DEBUG("'%s' added to list of active mediated devices used by '%s'", > + mdev->path, domname); > + } > + > + ret = 0; > + cleanup: > + virObjectUnlock(dst); > + return ret; > + > + rollback: > + for (j = 0; j < i; j++) { > + virMediatedDevicePtr tmp = virMediatedDeviceListGet(src, j); > + virMediatedDeviceListSteal(dst, tmp); > + } > + goto cleanup; > +} > diff --git a/src/util/virmdev.h b/src/util/virmdev.h > new file mode 100644 > index 0000000000..2f3d6bb840 > --- /dev/null > +++ b/src/util/virmdev.h > @@ -0,0 +1,123 @@ > +/* > + * virmdev.h: helper APIs for managing host mediated devices > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef __VIR_MDEV_H__ > +# define __VIR_MDEV_H__ > + > +# include "internal.h" > +# include "virobject.h" > +# include "virutil.h" > +# include "virpci.h" > + > +typedef enum { > + VIR_MDEV_MODEL_TYPE_VFIO_PCI = 0, > + > + VIR_MDEV_MODEL_TYPE_LAST > +} virMediatedDeviceModelType; > + > +VIR_ENUM_DECL(virMediatedDeviceModel) > + > + > +typedef struct _virMediatedDevice virMediatedDevice; > +typedef virMediatedDevice *virMediatedDevicePtr; > +typedef struct _virMediatedDeviceAddress virMediatedDeviceAddress; > +typedef virMediatedDeviceAddress *virMediatedDeviceAddressPtr; > +typedef struct _virMediatedDeviceList virMediatedDeviceList; > +typedef virMediatedDeviceList *virMediatedDeviceListPtr; > + > +typedef int (*virMediatedDeviceCallback)(virMediatedDevicePtr dev, > + const char *path, void *opaque); > + > +virMediatedDevicePtr > +virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model); > + > +virMediatedDevicePtr > +virMediatedDeviceCopy(virMediatedDevicePtr dev); > + > +void > +virMediatedDeviceFree(virMediatedDevicePtr dev); > + > +const char * > +virMediatedDeviceGetPath(virMediatedDevicePtr dev); > + > +void > +virMediatedDeviceGetUsedBy(virMediatedDevicePtr dev, > + const char **drvname, const char **domname); > + > +int > +virMediatedDeviceSetUsedBy(virMediatedDevicePtr dev, > + const char *drvname, > + const char *domname); > + > +char * > +virMediatedDeviceGetIOMMUGroupDev(virMediatedDevicePtr dev); > + > +int > +virMediatedDeviceGetIOMMUGroupNum(virMediatedDevicePtr dev); > + > +char * > +virMediatedDeviceGetSysfsPath(const char *uuidstr); > + > +bool > +virMediatedDeviceIsUsed(virMediatedDevicePtr dev, > + virMediatedDeviceListPtr list); > + > +bool > +virMediatedDeviceIsUsed(virMediatedDevicePtr dev, > + virMediatedDeviceListPtr list); > + > +virMediatedDeviceListPtr > +virMediatedDeviceListNew(void); > + > +int > +virMediatedDeviceListAdd(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev); > + > +virMediatedDevicePtr > +virMediatedDeviceListGet(virMediatedDeviceListPtr list, > + ssize_t idx); > + > +size_t > +virMediatedDeviceListCount(virMediatedDeviceListPtr list); > + > +virMediatedDevicePtr > +virMediatedDeviceListSteal(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev); > + > +virMediatedDevicePtr > +virMediatedDeviceListStealIndex(virMediatedDeviceListPtr list, > + ssize_t idx); > + > +void > +virMediatedDeviceListDel(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev); > + > +virMediatedDevicePtr > +virMediatedDeviceListFind(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev); > + > +int > +virMediatedDeviceListFindIndex(virMediatedDeviceListPtr list, > + virMediatedDevicePtr dev); > + > +int > +virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst, > + virMediatedDeviceListPtr src, > + const char *drvname, > + const char *domname); > +#endif /* __VIR_MDEV_H__ */ > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
