[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v1 12/31] util: Introduce virNVMeDevice module



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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]