[libvirt] [PATCH v1 12/31] util: Introduce virNVMeDevice module
Michal Privoznik
mprivozn at redhat.com
Fri Aug 2 15:19:54 UTC 2019
On 7/16/19 3:54 PM, Peter Krempa wrote:
> On Thu, Jul 11, 2019 at 17:53:59 +0200, Michal Privoznik wrote:
>> This module will be used by virHostdevManager and it's inspired
>> by virPCIDevice module. They are very similar except instead of
>> what makes a NVMe device: PCI address AND namespace ID. This
>> means that a NVMe device can appear in a domain multiple times,
>> each time with a different namespace.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/libvirt_private.syms | 18 ++
>> src/util/Makefile.inc.am | 2 +
>> src/util/virnvme.c | 412 +++++++++++++++++++++++++++++++++++++++
>> src/util/virnvme.h | 89 +++++++++
>> 4 files changed, 521 insertions(+)
>> create mode 100644 src/util/virnvme.c
>> create mode 100644 src/util/virnvme.h
>
> [...]
>
>> diff --git a/src/util/virnvme.c b/src/util/virnvme.c
>> new file mode 100644
>> index 0000000000..53724b63f7
>> --- /dev/null
>> +++ b/src/util/virnvme.c
>> @@ -0,0 +1,412 @@
>> +/*
>> + * virnvme.c: helper APIs for managing NVMe 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 "virnvme.h"
>> +#include "virobject.h"
>> +#include "virpci.h"
>> +#include "viralloc.h"
>> +#include "virlog.h"
>> +#include "virstring.h"
>> +
>> +VIR_LOG_INIT("util.pci");
>
> please use a different log domain
>
>> +#define VIR_FROM_THIS VIR_FROM_NONE
>> +
>> +struct _virNVMeDevice {
>> + virPCIDeviceAddress address; /* PCI address of controller */
>> + unsigned long namespace; /* Namespace ID */
>
> unsinged int/unsigned long long
>
>> + bool managed;
>> +
>> + char *drvname;
>> + char *domname;
>> +};
>> +
>> +
>> +struct _virNVMeDeviceList {
>> + virObjectLockable parent;
>> +
>> + size_t count;
>> + virNVMeDevicePtr *devs;
>> +};
>> +
>
> [...]
>
>
>> +int
>> +virNVMeDeviceListAdd(virNVMeDeviceListPtr list,
>> + const virNVMeDevice *dev)
>> +{
>> + virNVMeDevicePtr tmp;
>> +
>> + if ((tmp = virNVMeDeviceListLookup(list, dev))) {
>> + VIR_AUTOFREE(char *) addrStr = virPCIDeviceAddressAsString(&tmp->address);
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("NVMe device %s namespace %lu is already on the list"),
>> + NULLSTR(addrStr), tmp->namespace);
>> + return -1;
>> + }
>> +
>> + if (!(tmp = virNVMeDeviceCopy(dev)) ||
>> + VIR_APPEND_ELEMENT(list->devs, list->count, tmp) < 0) {
>> + virNVMeDeviceFree(tmp);
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +int
>> +virNVMeDeviceListDel(virNVMeDeviceListPtr list,
>> + const virNVMeDevice *dev)
>> +{
>> + ssize_t idx;
>> + virNVMeDevicePtr tmp = NULL;
>> +
>> + if ((idx = virNVMeDeviceListLookupIndex(list, dev)) < 0) {
>> + VIR_AUTOFREE(char *) addrStr = virPCIDeviceAddressAsString(&dev->address);
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("NVMe device %s namespace %lu not found"),
>> + NULLSTR(addrStr), dev->namespace);
>> + return -1;
>> + }
>> +
>> + tmp = list->devs[idx];
>> + VIR_DELETE_ELEMENT(list->devs, idx, list->count);
>> + virNVMeDeviceFree(tmp);
>> + return 0;
>> +}
>> +
>> +
>> +virNVMeDevicePtr
>> +virNVMeDeviceListGet(virNVMeDeviceListPtr list,
>> + size_t i)
>
> [1] (see below)
>
>> +{
>> + return i < list->count ? list->devs[i] : NULL;
>> +}
>> +
>> +
>> +virNVMeDevicePtr
>> +virNVMeDeviceListLookup(virNVMeDeviceListPtr list,
>> + const virNVMeDevice *dev)
>> +{
>> + ssize_t idx;
>> +
>> + if ((idx = virNVMeDeviceListLookupIndex(list, dev)) < 0)
>> + return NULL;
>> +
>> + return list->devs[idx];
>> +}
>> +
>> +
>> +ssize_t
>
> This function seems to be too unsafe to export as people might want to
> store the index while not holding the lock and something would then
> change it. Also [1] has the same issue.
This is not any different to other modules used by virhostdev. I agree
that they are unsafe, but at the same time I think rewriting them all
(to keep consistency) wouldn't result in cleaner code. Note that the
list is locked outside of this source file (is locked from within
virhostdev) - again, not something that complies with our rules, but it
makes sense IMO.
Note that all other APIs require locking from the caller (e.g.
virNVMeDeviceListAdd()).
I'll add a comment into the header file that virNVMeDeviceList is a
lockable object that requires caller to acquire the lock and hold it
throughout whole section involving it.
>
>> +virNVMeDeviceListLookupIndex(virNVMeDeviceListPtr list,
>> + const virNVMeDevice *dev)
>> +{
>> + size_t i;
>> +
>> + if (!list)
>> + return -1;
>> +
>> + for (i = 0; i < list->count; i++) {
>> + virNVMeDevicePtr other = list->devs[i];
>> +
>> + if (virPCIDeviceAddressEqual(&dev->address, &other->address) &&
>> + dev->namespace == other->namespace)
>> + return i;
>> + }
>> +
>> + return -1;
>> +}
>> +
>> +
>> +static virNVMeDevicePtr
>> +virNVMeDeviceListLookupByPCIAddress(virNVMeDeviceListPtr list,
>> + const virPCIDeviceAddress *address)
>> +{
>> + size_t i;
>> +
>> + if (!list)
>> + return NULL;
>> +
>> + for (i = 0; i < list->count; i++) {
>> + virNVMeDevicePtr other = list->devs[i];
>> +
>> + if (virPCIDeviceAddressEqual(address, &other->address))
>> + return other;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +
>> +virPCIDeviceListPtr
>> +virNVMeDeviceListCreateDetachList(virNVMeDeviceListPtr activeList,
>> + virNVMeDeviceListPtr toDetachList)
>> +{
>> + VIR_AUTOUNREF(virPCIDeviceListPtr) pciDevices = NULL;
>> + size_t i;
>> +
>> + if (!(pciDevices = virPCIDeviceListNew()))
>> + return NULL;
>> +
>> + for (i = 0; i < toDetachList->count; i++) {
>> + const virNVMeDevice *d = toDetachList->devs[i];
>> + VIR_AUTOPTR(virPCIDevice) pci = NULL;
>> +
>> + /* If there is a NVMe device with the same PCI address on
>> + * the activeList, the device is already detached. */
>> + if (virNVMeDeviceListLookupByPCIAddress(activeList, &d->address))
>> + continue;
>> +
>> + /* It may happen that we want to detach two namespaces
>> + * from the same NVMe device. This will be represented as
>> + * two different instances of virNVMeDevice, but
>> + * obviously we want to put the PCI device on the detach
>> + * list only once. */
>> + if (virPCIDeviceListFindByIDs(pciDevices,
>> + d->address.domain,
>> + d->address.bus,
>> + d->address.slot,
>> + d->address.function))
>> + continue;
>> +
>> + if (!(pci = virPCIDeviceNew(d->address.domain,
>> + d->address.bus,
>> + d->address.slot,
>> + d->address.function)))
>> + return NULL;
>> +
>> + /* NVMe devices must be bound to vfio */
>> + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO);
>> + virPCIDeviceSetManaged(pci, d->managed);
>> +
>> + if (virPCIDeviceListAdd(pciDevices, pci) < 0)
>> + return NULL;
>> +
>> + /* avoid freeing the device */
>> + pci = NULL;
>> + }
>> +
>> + VIR_RETURN_PTR(pciDevices);
>> +}
>> +
>> +
>> +virPCIDeviceListPtr
>> +virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
>
> This function is too complex to be without a comment describing it.
> Especially since it's returning list of pci devices.
Okay, I'll add a comment here too.
Michal
More information about the libvir-list
mailing list