[PATCH 0/9] reduce code duplication in NodeDevice driver
Hi, This series reduces code duplication between qemu_driver.c and libxl_driver.c by adding common code, related to NodeDevicePtr driver functions, into helper functions in domain_driver.c. No functional change was made. Daniel Henrique Barboza (9): qemu, libxl, hypervisor: use virDomainDriverNodeDeviceReset() helper datatypes.h: register AUTOPTR_CLEANUP_FUNC for virNodeDevicePtr domain_driver.c: use g_auto* in virDomainDriverNodeDeviceReset() qemu, libxl, hypervisor: use virDomainDriverNodeDeviceReAttach() helper domain_driver.c: use g_auto* in virDomainDriverNodeDeviceReAttach() libxl_driver.c: validate 'driverName' earlier in libxlNodeDeviceDetachFlags() qemu_driver.c: validate 'driverName' earlier in qemuNodeDeviceDetachFlags() qemu, libxl, hypervisor: use virDomainDriverNodeDeviceDetachFlags() helper domain_driver.c: use g_auto* in virDomainDriverNodeDeviceDetachFlags() src/datatypes.h | 2 + src/hypervisor/domain_driver.c | 147 +++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 11 +++ src/hypervisor/meson.build | 1 + src/libvirt_private.syms | 3 + src/libxl/libxl_driver.c | 164 +++------------------------------ src/qemu/qemu_driver.c | 164 ++++----------------------------- 7 files changed, 192 insertions(+), 300 deletions(-) -- 2.26.2
libxlNodeDeviceReset() and qemuNodeDeviceReset() are mostly equal, differing only how the virHostdevManager pointer is retrieved. Put the common code into virDomainDriverNodeDeviceReset() to reduce code duplication. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 58 ++++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 4 +++ src/hypervisor/meson.build | 1 + src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 53 ++----------------------------- src/qemu/qemu_driver.c | 49 ++-------------------------- 6 files changed, 70 insertions(+), 96 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 5b03f79833..0c86fd714f 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -25,6 +25,10 @@ #include "virstring.h" #include "vircrypto.h" #include "virutil.h" +#include "virhostdev.h" +#include "viraccessapicheck.h" +#include "datatypes.h" +#include "driver.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -365,3 +369,57 @@ virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, return 0; } + + +int +virDomainDriverNodeDeviceReset(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr) +{ + virPCIDevicePtr pci; + virPCIDeviceAddress devAddr; + virNodeDeviceDefPtr def = NULL; + g_autofree char *xml = NULL; + virConnectPtr nodeconn = NULL; + virNodeDevicePtr nodedev = NULL; + int ret = -1; + + if (!(nodeconn = virGetConnectNodeDev())) + goto cleanup; + + /* 'dev' is associated with virConnectPtr, so for split + * daemons, we need to get a copy that is associated with + * the virnodedevd daemon. */ + if (!(nodedev = virNodeDeviceLookupByName( + nodeconn, virNodeDeviceGetName(dev)))) + goto cleanup; + + xml = virNodeDeviceGetXMLDesc(nodedev, 0); + if (!xml) + goto cleanup; + + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + if (!def) + goto cleanup; + + /* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ + if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) + goto cleanup; + + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) + goto cleanup; + + pci = virPCIDeviceNew(&devAddr); + if (!pci) + goto cleanup; + + ret = virHostdevPCINodeDeviceReset(hostdevMgr, pci); + + virPCIDeviceFree(pci); + cleanup: + virNodeDeviceDefFree(def); + virObjectUnref(nodedev); + virObjectUnref(nodeconn); + return ret; + +} diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index 86b92d0284..b690844fe5 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -22,6 +22,7 @@ #include "domain_conf.h" #include "node_device_conf.h" +#include "virhostdev.h" char * virDomainDriverGenerateRootHash(const char *drivername, @@ -49,3 +50,6 @@ int virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef, int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, virPCIDeviceAddressPtr devAddr); + +int virDomainDriverNodeDeviceReset(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr); diff --git a/src/hypervisor/meson.build b/src/hypervisor/meson.build index 85149c683e..32d5ab365f 100644 --- a/src/hypervisor/meson.build +++ b/src/hypervisor/meson.build @@ -11,6 +11,7 @@ hypervisor_lib = static_library( hypervisor_sources, ], dependencies: [ + access_dep, src_dep, ], include_directories: [ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8138780237..1f6048e3f7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1504,6 +1504,7 @@ virDomainDriverGenerateMachineName; virDomainDriverGenerateRootHash; virDomainDriverMergeBlkioDevice; virDomainDriverNodeDeviceGetPCIInfo; +virDomainDriverNodeDeviceReset; virDomainDriverParseBlkioDeviceStr; virDomainDriverSetupPersistentDefBlkioParams; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3eaf106006..50baeb43b6 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5910,59 +5910,12 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) static int libxlNodeDeviceReset(virNodeDevicePtr dev) { - virPCIDevicePtr pci = NULL; - virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; - char *xml = NULL; libxlDriverPrivatePtr driver = dev->conn->privateData; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; - - if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; - - /* 'dev' is associated with the QEMU virConnectPtr, - * so for split daemons, we need to get a copy that - * is associated with the virnodedevd daemon. - */ - if (!(nodedev = virNodeDeviceLookupByName( - nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; - - xml = virNodeDeviceGetXMLDesc(nodedev, 0); - if (!xml) - goto cleanup; - - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); - if (!def) - goto cleanup; - - /* ACL check must happen against original 'dev', - * not the new 'nodedev' we acquired */ - if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) - goto cleanup; - - if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; - - pci = virPCIDeviceNew(&devAddr); - if (!pci) - goto cleanup; - - if (virHostdevPCINodeDeviceReset(hostdev_mgr, pci) < 0) - goto cleanup; - ret = 0; - - cleanup: - virPCIDeviceFree(pci); - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - VIR_FREE(xml); - return ret; + /* virNodeDeviceResetEnsureACL() is being called by + * virDomainDriverNodeDeviceReset() */ + return virDomainDriverNodeDeviceReset(dev, hostdev_mgr); } static char * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed840a5c8d..8270a26c0b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12108,54 +12108,11 @@ static int qemuNodeDeviceReset(virNodeDevicePtr dev) { virQEMUDriverPtr driver = dev->conn->privateData; - virPCIDevicePtr pci; - virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; - g_autofree char *xml = NULL; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; - - if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; - - /* 'dev' is associated with the QEMU virConnectPtr, - * so for split daemons, we need to get a copy that - * is associated with the virnodedevd daemon. - */ - if (!(nodedev = virNodeDeviceLookupByName( - nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; - xml = virNodeDeviceGetXMLDesc(nodedev, 0); - if (!xml) - goto cleanup; - - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); - if (!def) - goto cleanup; - - /* ACL check must happen against original 'dev', - * not the new 'nodedev' we acquired */ - if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) - goto cleanup; - - if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; - - pci = virPCIDeviceNew(&devAddr); - if (!pci) - goto cleanup; - - ret = virHostdevPCINodeDeviceReset(hostdev_mgr, pci); - - virPCIDeviceFree(pci); - cleanup: - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - return ret; + /* virNodeDeviceResetEnsureACL() is being called by + * virDomainDriverNodeDeviceReset() */ + return virDomainDriverNodeDeviceReset(dev, hostdev_mgr); } static int -- 2.26.2
Next patch will use g_autoptr() with virNodeDevicePtr for cleanups. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/datatypes.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/datatypes.h b/src/datatypes.h index ade3779e43..7a88aba0df 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -707,6 +707,8 @@ struct _virNodeDevice { char *parentName; /* parent device name */ }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNodeDevice, virObjectUnref); + /** * _virSecret: * -- 2.26.2
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 0c86fd714f..82e5587a50 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -375,51 +375,42 @@ int virDomainDriverNodeDeviceReset(virNodeDevicePtr dev, virHostdevManagerPtr hostdevMgr) { - virPCIDevicePtr pci; + g_autoptr(virPCIDevice) pci = NULL; virPCIDeviceAddress devAddr; - virNodeDeviceDefPtr def = NULL; + g_autoptr(virNodeDeviceDef) def = NULL; g_autofree char *xml = NULL; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; - int ret = -1; + g_autoptr(virConnect) nodeconn = NULL; + g_autoptr(virNodeDevice) nodedev = NULL; if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; + return -1; /* 'dev' is associated with virConnectPtr, so for split * daemons, we need to get a copy that is associated with * the virnodedevd daemon. */ if (!(nodedev = virNodeDeviceLookupByName( nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; + return -1; xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) - goto cleanup; + return -1; def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); if (!def) - goto cleanup; + return -1; /* ACL check must happen against original 'dev', * not the new 'nodedev' we acquired */ if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) - goto cleanup; + return -1; if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; + return -1; pci = virPCIDeviceNew(&devAddr); if (!pci) - goto cleanup; - - ret = virHostdevPCINodeDeviceReset(hostdevMgr, pci); - - virPCIDeviceFree(pci); - cleanup: - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - return ret; + return -1; + return virHostdevPCINodeDeviceReset(hostdevMgr, pci); } -- 2.26.2
libxlNodeDeviceReAttach() and qemuNodeDeviceReAttach() are mostly equal, differing only how the virHostdevManager pointer is retrieved. Put the common code into virDomainDriverNodeDeviceReAttach() to reduce code duplication. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 53 ++++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 3 ++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 53 ++-------------------------------- src/qemu/qemu_driver.c | 49 ++----------------------------- 5 files changed, 63 insertions(+), 96 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 82e5587a50..c559f94348 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -414,3 +414,56 @@ virDomainDriverNodeDeviceReset(virNodeDevicePtr dev, return virHostdevPCINodeDeviceReset(hostdevMgr, pci); } + + +int +virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr) +{ + virPCIDevicePtr pci = NULL; + virPCIDeviceAddress devAddr; + int ret = -1; + virNodeDeviceDefPtr def = NULL; + g_autofree char *xml = NULL; + virConnectPtr nodeconn = NULL; + virNodeDevicePtr nodedev = NULL; + + if (!(nodeconn = virGetConnectNodeDev())) + goto cleanup; + + /* 'dev' is associated with virConnectPtr, so for split + * daemons, we need to get a copy that is associated with + * the virnodedevd daemon. */ + if (!(nodedev = virNodeDeviceLookupByName( + nodeconn, virNodeDeviceGetName(dev)))) + goto cleanup; + + xml = virNodeDeviceGetXMLDesc(nodedev, 0); + if (!xml) + goto cleanup; + + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + if (!def) + goto cleanup; + + /* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ + if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) + goto cleanup; + + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) + goto cleanup; + + pci = virPCIDeviceNew(&devAddr); + if (!pci) + goto cleanup; + + ret = virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); + + virPCIDeviceFree(pci); + cleanup: + virNodeDeviceDefFree(def); + virObjectUnref(nodedev); + virObjectUnref(nodeconn); + return ret; +} diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index b690844fe5..71eed6d5a9 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -53,3 +53,6 @@ int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, int virDomainDriverNodeDeviceReset(virNodeDevicePtr dev, virHostdevManagerPtr hostdevMgr); + +int virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1f6048e3f7..ed01f79106 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1504,6 +1504,7 @@ virDomainDriverGenerateMachineName; virDomainDriverGenerateRootHash; virDomainDriverMergeBlkioDevice; virDomainDriverNodeDeviceGetPCIInfo; +virDomainDriverNodeDeviceReAttach; virDomainDriverNodeDeviceReset; virDomainDriverParseBlkioDeviceStr; virDomainDriverSetupPersistentDefBlkioParams; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 50baeb43b6..718717f14b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5852,59 +5852,12 @@ libxlNodeDeviceDettach(virNodeDevicePtr dev) static int libxlNodeDeviceReAttach(virNodeDevicePtr dev) { - virPCIDevicePtr pci = NULL; - virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; - char *xml = NULL; libxlDriverPrivatePtr driver = dev->conn->privateData; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; - - if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; - - /* 'dev' is associated with the QEMU virConnectPtr, - * so for split daemons, we need to get a copy that - * is associated with the virnodedevd daemon. - */ - if (!(nodedev = virNodeDeviceLookupByName( - nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; - - xml = virNodeDeviceGetXMLDesc(nodedev, 0); - if (!xml) - goto cleanup; - - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); - if (!def) - goto cleanup; - - /* ACL check must happen against original 'dev', - * not the new 'nodedev' we acquired */ - if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) - goto cleanup; - - if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; - - pci = virPCIDeviceNew(&devAddr); - if (!pci) - goto cleanup; - if (virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virPCIDeviceFree(pci); - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - VIR_FREE(xml); - return ret; + /* virNodeDeviceReAttachEnsureACL() is being called by + * virDomainDriverNodeDeviceReAttach() */ + return virDomainDriverNodeDeviceReAttach(dev, hostdev_mgr); } static int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8270a26c0b..64ae8fafc0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12054,54 +12054,11 @@ static int qemuNodeDeviceReAttach(virNodeDevicePtr dev) { virQEMUDriverPtr driver = dev->conn->privateData; - virPCIDevicePtr pci = NULL; - virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; - g_autofree char *xml = NULL; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; - - if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; - - /* 'dev' is associated with the QEMU virConnectPtr, - * so for split daemons, we need to get a copy that - * is associated with the virnodedevd daemon. - */ - if (!(nodedev = virNodeDeviceLookupByName( - nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; - - xml = virNodeDeviceGetXMLDesc(nodedev, 0); - if (!xml) - goto cleanup; - - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); - if (!def) - goto cleanup; - /* ACL check must happen against original 'dev', - * not the new 'nodedev' we acquired */ - if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) - goto cleanup; - - if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; - - pci = virPCIDeviceNew(&devAddr); - if (!pci) - goto cleanup; - - ret = virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci); - - virPCIDeviceFree(pci); - cleanup: - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - return ret; + /* virNodeDeviceReAttachEnsureACL() is being called by + * virDomainDriverNodeDeviceReAttach() */ + return virDomainDriverNodeDeviceReAttach(dev, hostdev_mgr); } static int -- 2.26.2
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index c559f94348..ea4c3c9466 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -420,50 +420,42 @@ int virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev, virHostdevManagerPtr hostdevMgr) { - virPCIDevicePtr pci = NULL; + g_autoptr(virPCIDevice) pci = NULL; virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; + g_autoptr(virNodeDeviceDef) def = NULL; g_autofree char *xml = NULL; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; + g_autoptr(virConnect) nodeconn = NULL; + g_autoptr(virNodeDevice) nodedev = NULL; if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; + return -1; /* 'dev' is associated with virConnectPtr, so for split * daemons, we need to get a copy that is associated with * the virnodedevd daemon. */ if (!(nodedev = virNodeDeviceLookupByName( nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; + return -1; xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) - goto cleanup; + return -1; def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); if (!def) - goto cleanup; + return -1; /* ACL check must happen against original 'dev', * not the new 'nodedev' we acquired */ if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) - goto cleanup; + return -1; if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; + return -1; pci = virPCIDeviceNew(&devAddr); if (!pci) - goto cleanup; - - ret = virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); + return -1; - virPCIDeviceFree(pci); - cleanup: - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - return ret; + return virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); } -- 2.26.2
The validation of 'driverName' does not depend on any other state and can be done right on the start of the function. We can fail earlier while avoiding a cleanup jump. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/libxl/libxl_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 718717f14b..7c7eeb3ad0 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5791,6 +5791,12 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, virCheckFlags(0, -1); + if (driverName && STRNEQ(driverName, "xen")) { + virReportError(VIR_ERR_INVALID_ARG, + _("unsupported driver name '%s'"), driverName); + return -1; + } + if (!(nodeconn = virGetConnectNodeDev())) goto cleanup; @@ -5822,13 +5828,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, if (!pci) goto cleanup; - if (!driverName || STREQ(driverName, "xen")) { - virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); - } else { - virReportError(VIR_ERR_INVALID_ARG, - _("unsupported driver name '%s'"), driverName); - goto cleanup; - } + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); if (virHostdevPCINodeDeviceDetach(hostdev_mgr, pci) < 0) goto cleanup; -- 2.26.2
The validation of 'driverName' does not depend on any other state and can be done right on the start of the function. We can fail earlier while avoiding a cleanup jump. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_driver.c | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 64ae8fafc0..c6ba33e4ad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11982,6 +11982,25 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, virCheckFlags(0, -1); + if (!driverName) + driverName = "vfio"; + + if (STREQ(driverName, "vfio") && !vfio) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("VFIO device assignment is currently not " + "supported on this system")); + return -1; + } else if (STREQ(driverName, "kvm")) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("KVM device assignment is no longer " + "supported on this system")); + return -1; + } else { + virReportError(VIR_ERR_INVALID_ARG, + _("unknown driver name '%s'"), driverName); + return -1; + } + if (!(nodeconn = virGetConnectNodeDev())) goto cleanup; @@ -12013,27 +12032,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, if (!pci) goto cleanup; - if (!driverName) - driverName = "vfio"; - - if (STREQ(driverName, "vfio")) { - if (!vfio) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("VFIO device assignment is currently not " - "supported on this system")); - goto cleanup; - } - virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); - } else if (STREQ(driverName, "kvm")) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("KVM device assignment is no longer " - "supported on this system")); - goto cleanup; - } else { - virReportError(VIR_ERR_INVALID_ARG, - _("unknown driver name '%s'"), driverName); - goto cleanup; - } + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); ret = virHostdevPCINodeDeviceDetach(hostdev_mgr, pci); cleanup: -- 2.26.2
libxlNodeDeviceDetachFlags() and qemuNodeDeviceDetachFlags() are mostly equal, aside from how the virHostdevmanager pointer is retrieved and the PCI stub driver used. Now that the PCI stub driver verification is done early in both functions, we can use the virDomainDriverNodeDeviceDetachFlags() helper to reduce code duplication between them. 'driverName' is checked inside the helper to set the appropriate stub driver. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 60 ++++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 4 +++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 54 ++---------------------------- src/qemu/qemu_driver.c | 49 ++------------------------- 5 files changed, 71 insertions(+), 97 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index ea4c3c9466..6ee74d6dff 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -459,3 +459,63 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev, return virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); } + +int +virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr, + const char *driverName) +{ + virPCIDevicePtr pci = NULL; + virPCIDeviceAddress devAddr; + int ret = -1; + virNodeDeviceDefPtr def = NULL; + g_autofree char *xml = NULL; + virConnectPtr nodeconn = NULL; + virNodeDevicePtr nodedev = NULL; + + if (!driverName) + return -1; + + if (!(nodeconn = virGetConnectNodeDev())) + goto cleanup; + + /* 'dev' is associated with virConnectPtr, so for split + * daemons, we need to get a copy that is associated with + * the virnodedevd daemon. */ + if (!(nodedev = virNodeDeviceLookupByName(nodeconn, + virNodeDeviceGetName(dev)))) + goto cleanup; + + xml = virNodeDeviceGetXMLDesc(nodedev, 0); + if (!xml) + goto cleanup; + + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + if (!def) + goto cleanup; + + /* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ + if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) + goto cleanup; + + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) + goto cleanup; + + pci = virPCIDeviceNew(&devAddr); + if (!pci) + goto cleanup; + + if (STREQ(driverName, "vfio")) + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); + else if (STREQ(driverName, "xen")) + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); + + ret = virHostdevPCINodeDeviceDetach(hostdevMgr, pci); + cleanup: + virPCIDeviceFree(pci); + virNodeDeviceDefFree(def); + virObjectUnref(nodedev); + virObjectUnref(nodeconn); + return ret; +} diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index 71eed6d5a9..a22a3ee76c 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -56,3 +56,7 @@ int virDomainDriverNodeDeviceReset(virNodeDevicePtr dev, int virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev, virHostdevManagerPtr hostdevMgr); + +int virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr, + const char *driverName); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ed01f79106..57622dc9a7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1503,6 +1503,7 @@ virDomainCgroupSetupMemtune; virDomainDriverGenerateMachineName; virDomainDriverGenerateRootHash; virDomainDriverMergeBlkioDevice; +virDomainDriverNodeDeviceDetachFlags; virDomainDriverNodeDeviceGetPCIInfo; virDomainDriverNodeDeviceReAttach; virDomainDriverNodeDeviceReset; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7c7eeb3ad0..4fc59b2a7e 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5779,15 +5779,8 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, const char *driverName, unsigned int flags) { - virPCIDevicePtr pci = NULL; - virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; - char *xml = NULL; libxlDriverPrivatePtr driver = dev->conn->privateData; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; virCheckFlags(0, -1); @@ -5797,50 +5790,9 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, return -1; } - if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; - - /* 'dev' is associated with the QEMU virConnectPtr, - * so for split daemons, we need to get a copy that - * is associated with the virnodedevd daemon. - */ - if (!(nodedev = virNodeDeviceLookupByName(nodeconn, - virNodeDeviceGetName(dev)))) - goto cleanup; - - xml = virNodeDeviceGetXMLDesc(nodedev, 0); - if (!xml) - goto cleanup; - - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); - if (!def) - goto cleanup; - - /* ACL check must happen against original 'dev', - * not the new 'nodedev' we acquired */ - if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) - goto cleanup; - - if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; - - pci = virPCIDeviceNew(&devAddr); - if (!pci) - goto cleanup; - - virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); - - if (virHostdevPCINodeDeviceDetach(hostdev_mgr, pci) < 0) - goto cleanup; - - ret = 0; - cleanup: - virPCIDeviceFree(pci); - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - VIR_FREE(xml); - return ret; + /* virNodeDeviceDetachFlagsEnsureACL() is being called by + * virDomainDriverNodeDeviceDetachFlags() */ + return virDomainDriverNodeDeviceDetachFlags(dev, hostdev_mgr, driverName); } static int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c6ba33e4ad..21ce143764 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11970,15 +11970,8 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, unsigned int flags) { virQEMUDriverPtr driver = dev->conn->privateData; - virPCIDevicePtr pci = NULL; - virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; - g_autofree char *xml = NULL; bool vfio = qemuHostdevHostSupportsPassthroughVFIO(); virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; virCheckFlags(0, -1); @@ -12001,46 +11994,10 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, return -1; } - if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; - - /* 'dev' is associated with the QEMU virConnectPtr, - * so for split daemons, we need to get a copy that - * is associated with the virnodedevd daemon. - */ - if (!(nodedev = virNodeDeviceLookupByName(nodeconn, - virNodeDeviceGetName(dev)))) - goto cleanup; - - xml = virNodeDeviceGetXMLDesc(nodedev, 0); - if (!xml) - goto cleanup; - - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); - if (!def) - goto cleanup; - /* ACL check must happen against original 'dev', - * not the new 'nodedev' we acquired */ - if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) - goto cleanup; - - if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; - - pci = virPCIDeviceNew(&devAddr); - if (!pci) - goto cleanup; - - virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); - - ret = virHostdevPCINodeDeviceDetach(hostdev_mgr, pci); - cleanup: - virPCIDeviceFree(pci); - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - return ret; + /* virNodeDeviceDetachFlagsEnsureACL() is being called by + * virDomainDriverNodeDeviceDetachFlags() */ + return virDomainDriverNodeDeviceDetachFlags(dev, hostdev_mgr, driverName); } static int -- 2.26.2
On a Monday in 2021, Daniel Henrique Barboza wrote:
libxlNodeDeviceDetachFlags() and qemuNodeDeviceDetachFlags() are mostly equal, aside from how the virHostdevmanager pointer is retrieved and the PCI stub driver used.
Now that the PCI stub driver verification is done early in both functions, we can use the virDomainDriverNodeDeviceDetachFlags() helper to reduce code duplication between them. 'driverName' is checked inside the helper to set the appropriate stub driver.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 60 ++++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 4 +++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 54 ++---------------------------- src/qemu/qemu_driver.c | 49 ++------------------------- 5 files changed, 71 insertions(+), 97 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index ea4c3c9466..6ee74d6dff 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -459,3 +459,63 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev,
return virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); } + +int +virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr, + const char *driverName)
This helper does not even take flags, so the only reason to keep the 'Flags' in its name is to be consistent with the driver method it implements...
+{ + virPCIDevicePtr pci = NULL; + virPCIDeviceAddress devAddr; + int ret = -1; + virNodeDeviceDefPtr def = NULL; + g_autofree char *xml = NULL; + virConnectPtr nodeconn = NULL; + virNodeDevicePtr nodedev = NULL; + + if (!driverName) + return -1; + + if (!(nodeconn = virGetConnectNodeDev())) + goto cleanup; + + /* 'dev' is associated with virConnectPtr, so for split + * daemons, we need to get a copy that is associated with + * the virnodedevd daemon. */ + if (!(nodedev = virNodeDeviceLookupByName(nodeconn, + virNodeDeviceGetName(dev)))) + goto cleanup; + + xml = virNodeDeviceGetXMLDesc(nodedev, 0); + if (!xml) + goto cleanup; + + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + if (!def) + goto cleanup; + + /* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ + if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) + goto cleanup; +
... and the ACL check required. But moving the ACL checks into src/hypervisor means they are no longer covered by the check-aclrules check. I'd rather keep the ACL checks repetitive in each driver than risk the chance of missing them, but that invalidates most of these refactors. Any ideas? Jano
+ if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) + goto cleanup; + + pci = virPCIDeviceNew(&devAddr); + if (!pci) + goto cleanup; + + if (STREQ(driverName, "vfio")) + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); + else if (STREQ(driverName, "xen")) + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); + + ret = virHostdevPCINodeDeviceDetach(hostdevMgr, pci); + cleanup: + virPCIDeviceFree(pci); + virNodeDeviceDefFree(def); + virObjectUnref(nodedev); + virObjectUnref(nodeconn); + return ret; +}
On Tue, Feb 02, 2021 at 05:18:51PM +0100, Ján Tomko wrote:
On a Monday in 2021, Daniel Henrique Barboza wrote:
libxlNodeDeviceDetachFlags() and qemuNodeDeviceDetachFlags() are mostly equal, aside from how the virHostdevmanager pointer is retrieved and the PCI stub driver used.
Now that the PCI stub driver verification is done early in both functions, we can use the virDomainDriverNodeDeviceDetachFlags() helper to reduce code duplication between them. 'driverName' is checked inside the helper to set the appropriate stub driver.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 60 ++++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 4 +++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 54 ++---------------------------- src/qemu/qemu_driver.c | 49 ++------------------------- 5 files changed, 71 insertions(+), 97 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index ea4c3c9466..6ee74d6dff 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -459,3 +459,63 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev,
return virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); } + +int +virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr, + const char *driverName)
This helper does not even take flags, so the only reason to keep the 'Flags' in its name is to be consistent with the driver method it implements...
+{ + virPCIDevicePtr pci = NULL; + virPCIDeviceAddress devAddr; + int ret = -1; + virNodeDeviceDefPtr def = NULL; + g_autofree char *xml = NULL; + virConnectPtr nodeconn = NULL; + virNodeDevicePtr nodedev = NULL; + + if (!driverName) + return -1; + + if (!(nodeconn = virGetConnectNodeDev())) + goto cleanup; + + /* 'dev' is associated with virConnectPtr, so for split + * daemons, we need to get a copy that is associated with + * the virnodedevd daemon. */ + if (!(nodedev = virNodeDeviceLookupByName(nodeconn, + virNodeDeviceGetName(dev)))) + goto cleanup; + + xml = virNodeDeviceGetXMLDesc(nodedev, 0); + if (!xml) + goto cleanup; + + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + if (!def) + goto cleanup; + + /* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ + if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) + goto cleanup; +
... and the ACL check required.
But moving the ACL checks into src/hypervisor means they are no longer covered by the check-aclrules check.
I'd rather keep the ACL checks repetitive in each driver than risk the chance of missing them, but that invalidates most of these refactors.
Any ideas?
Make the check-aclrules also validate this new source file ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On a Tuesday in 2021, Daniel P. Berrangé wrote:
On Tue, Feb 02, 2021 at 05:18:51PM +0100, Ján Tomko wrote:
On a Monday in 2021, Daniel Henrique Barboza wrote:
libxlNodeDeviceDetachFlags() and qemuNodeDeviceDetachFlags() are mostly equal, aside from how the virHostdevmanager pointer is retrieved and the PCI stub driver used.
Now that the PCI stub driver verification is done early in both functions, we can use the virDomainDriverNodeDeviceDetachFlags() helper to reduce code duplication between them. 'driverName' is checked inside the helper to set the appropriate stub driver.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 60 ++++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 4 +++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 54 ++---------------------------- src/qemu/qemu_driver.c | 49 ++------------------------- 5 files changed, 71 insertions(+), 97 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index ea4c3c9466..6ee74d6dff 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -459,3 +459,63 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev,
return virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); } + +int +virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr, + const char *driverName)
This helper does not even take flags, so the only reason to keep the 'Flags' in its name is to be consistent with the driver method it implements...
+{ + virPCIDevicePtr pci = NULL; + virPCIDeviceAddress devAddr; + int ret = -1; + virNodeDeviceDefPtr def = NULL; + g_autofree char *xml = NULL; + virConnectPtr nodeconn = NULL; + virNodeDevicePtr nodedev = NULL; + + if (!driverName) + return -1; + + if (!(nodeconn = virGetConnectNodeDev())) + goto cleanup; + + /* 'dev' is associated with virConnectPtr, so for split + * daemons, we need to get a copy that is associated with + * the virnodedevd daemon. */ + if (!(nodedev = virNodeDeviceLookupByName(nodeconn, + virNodeDeviceGetName(dev)))) + goto cleanup; + + xml = virNodeDeviceGetXMLDesc(nodedev, 0); + if (!xml) + goto cleanup; + + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + if (!def) + goto cleanup; + + /* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ + if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) + goto cleanup; +
... and the ACL check required.
But moving the ACL checks into src/hypervisor means they are no longer covered by the check-aclrules check.
I'd rather keep the ACL checks repetitive in each driver than risk the chance of missing them, but that invalidates most of these refactors.
Any ideas?
Make the check-aclrules also validate this new source file ?
Ah, I thought they also verify that each driver method has at least one ACL call, but we have plenty of methods that are just wrappers for MethodFlags(..., 0); Jano
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Feb 02, 2021 at 05:32:14PM +0100, Ján Tomko wrote:
On a Tuesday in 2021, Daniel P. Berrangé wrote:
On Tue, Feb 02, 2021 at 05:18:51PM +0100, Ján Tomko wrote:
On a Monday in 2021, Daniel Henrique Barboza wrote:
libxlNodeDeviceDetachFlags() and qemuNodeDeviceDetachFlags() are mostly equal, aside from how the virHostdevmanager pointer is retrieved and the PCI stub driver used.
Now that the PCI stub driver verification is done early in both functions, we can use the virDomainDriverNodeDeviceDetachFlags() helper to reduce code duplication between them. 'driverName' is checked inside the helper to set the appropriate stub driver.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 60 ++++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 4 +++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 54 ++---------------------------- src/qemu/qemu_driver.c | 49 ++------------------------- 5 files changed, 71 insertions(+), 97 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index ea4c3c9466..6ee74d6dff 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -459,3 +459,63 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev,
return virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); } + +int +virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr, + const char *driverName)
This helper does not even take flags, so the only reason to keep the 'Flags' in its name is to be consistent with the driver method it implements...
+{ + virPCIDevicePtr pci = NULL; + virPCIDeviceAddress devAddr; + int ret = -1; + virNodeDeviceDefPtr def = NULL; + g_autofree char *xml = NULL; + virConnectPtr nodeconn = NULL; + virNodeDevicePtr nodedev = NULL; + + if (!driverName) + return -1; + + if (!(nodeconn = virGetConnectNodeDev())) + goto cleanup; + + /* 'dev' is associated with virConnectPtr, so for split + * daemons, we need to get a copy that is associated with + * the virnodedevd daemon. */ + if (!(nodedev = virNodeDeviceLookupByName(nodeconn, + virNodeDeviceGetName(dev)))) + goto cleanup; + + xml = virNodeDeviceGetXMLDesc(nodedev, 0); + if (!xml) + goto cleanup; + + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + if (!def) + goto cleanup; + + /* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ + if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) + goto cleanup; +
... and the ACL check required.
But moving the ACL checks into src/hypervisor means they are no longer covered by the check-aclrules check.
I'd rather keep the ACL checks repetitive in each driver than risk the chance of missing them, but that invalidates most of these refactors.
Any ideas?
Make the check-aclrules also validate this new source file ?
Ah, I thought they also verify that each driver method has at least one ACL call, but we have plenty of methods that are just wrappers for MethodFlags(..., 0);
The check-aclrules.py script has some logic which looks to see if the method contains a function call that resolves to another public API entrypoint, as a special case. So basically we'll need to process the hypervisor_driver.c file to extract a list of public API entrpoints with ACL checks, and then in special case the real drivers if they call one of these shared functions. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/2/21 1:35 PM, Daniel P. Berrangé wrote:
On Tue, Feb 02, 2021 at 05:32:14PM +0100, Ján Tomko wrote:
On a Tuesday in 2021, Daniel P. Berrangé wrote:
On Tue, Feb 02, 2021 at 05:18:51PM +0100, Ján Tomko wrote:
On a Monday in 2021, Daniel Henrique Barboza wrote:
libxlNodeDeviceDetachFlags() and qemuNodeDeviceDetachFlags() are mostly equal, aside from how the virHostdevmanager pointer is retrieved and the PCI stub driver used.
Now that the PCI stub driver verification is done early in both functions, we can use the virDomainDriverNodeDeviceDetachFlags() helper to reduce code duplication between them. 'driverName' is checked inside the helper to set the appropriate stub driver.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 60 ++++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 4 +++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 54 ++---------------------------- src/qemu/qemu_driver.c | 49 ++------------------------- 5 files changed, 71 insertions(+), 97 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index ea4c3c9466..6ee74d6dff 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -459,3 +459,63 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev,
return virHostdevPCINodeDeviceReAttach(hostdevMgr, pci); } + +int +virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, + virHostdevManagerPtr hostdevMgr, + const char *driverName)
This helper does not even take flags, so the only reason to keep the 'Flags' in its name is to be consistent with the driver method it implements...
Fair point. I can remove the 'Flags' name of the helper since the flags are now being considered by the callers.
+{ + virPCIDevicePtr pci = NULL; + virPCIDeviceAddress devAddr; + int ret = -1; + virNodeDeviceDefPtr def = NULL; + g_autofree char *xml = NULL; + virConnectPtr nodeconn = NULL; + virNodeDevicePtr nodedev = NULL; + + if (!driverName) + return -1; + + if (!(nodeconn = virGetConnectNodeDev())) + goto cleanup; + + /* 'dev' is associated with virConnectPtr, so for split + * daemons, we need to get a copy that is associated with + * the virnodedevd daemon. */ + if (!(nodedev = virNodeDeviceLookupByName(nodeconn, + virNodeDeviceGetName(dev)))) + goto cleanup; + + xml = virNodeDeviceGetXMLDesc(nodedev, 0); + if (!xml) + goto cleanup; + + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + if (!def) + goto cleanup; + + /* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ + if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) + goto cleanup; +
... and the ACL check required.
But moving the ACL checks into src/hypervisor means they are no longer covered by the check-aclrules check.
I'd rather keep the ACL checks repetitive in each driver than risk the chance of missing them, but that invalidates most of these refactors.
Any ideas?
Make the check-aclrules also validate this new source file ?
I appreciate any pointers in doing that. I haven't found any 'not horrible' way (e.g. creating a new condition to add domain_driver.c as a valid driver implementation file) of doing it in check-aclrules.py. Thanks, DHB
Ah, I thought they also verify that each driver method has at least one ACL call, but we have plenty of methods that are just wrappers for MethodFlags(..., 0);
The check-aclrules.py script has some logic which looks to see if the method contains a function call that resolves to another public API entrypoint, as a special case.
So basically we'll need to process the hypervisor_driver.c file to extract a list of public API entrpoints with ACL checks, and then in special case the real drivers if they call one of these shared functions.
Regards, Daniel
On 2/2/21 1:35 PM, Daniel P. Berrangé wrote:
The check-aclrules.py script has some logic which looks to see if the method contains a function call that resolves to another public API entrypoint, as a special case.
So basically we'll need to process the hypervisor_driver.c file to extract a list of public API entrpoints with ACL checks, and then in special case the real drivers if they call one of these shared functions.
I'll give it a shot. Thanks, DHB
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 6ee74d6dff..c08b7d46c5 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -465,57 +465,50 @@ virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, virHostdevManagerPtr hostdevMgr, const char *driverName) { - virPCIDevicePtr pci = NULL; + g_autoptr(virPCIDevice) pci = NULL; virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; + g_autoptr(virNodeDeviceDef) def = NULL; g_autofree char *xml = NULL; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; + g_autoptr(virConnect) nodeconn = NULL; + g_autoptr(virNodeDevice) nodedev = NULL; if (!driverName) return -1; if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; + return -1; /* 'dev' is associated with virConnectPtr, so for split * daemons, we need to get a copy that is associated with * the virnodedevd daemon. */ if (!(nodedev = virNodeDeviceLookupByName(nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; + return -1; xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) - goto cleanup; + return -1; def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); if (!def) - goto cleanup; + return -1; /* ACL check must happen against original 'dev', * not the new 'nodedev' we acquired */ if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) - goto cleanup; + return -1; if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; + return -1; pci = virPCIDeviceNew(&devAddr); if (!pci) - goto cleanup; + return -1; if (STREQ(driverName, "vfio")) virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); else if (STREQ(driverName, "xen")) virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); - ret = virHostdevPCINodeDeviceDetach(hostdevMgr, pci); - cleanup: - virPCIDeviceFree(pci); - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - return ret; + return virHostdevPCINodeDeviceDetach(hostdevMgr, pci); } -- 2.26.2
On a Monday in 2021, Daniel Henrique Barboza wrote:
Hi,
This series reduces code duplication between qemu_driver.c and libxl_driver.c by adding common code, related to NodeDevicePtr driver functions, into helper functions in domain_driver.c.
No functional change was made.
Daniel Henrique Barboza (9): qemu, libxl, hypervisor: use virDomainDriverNodeDeviceReset() helper datatypes.h: register AUTOPTR_CLEANUP_FUNC for virNodeDevicePtr domain_driver.c: use g_auto* in virDomainDriverNodeDeviceReset() qemu, libxl, hypervisor: use virDomainDriverNodeDeviceReAttach() helper domain_driver.c: use g_auto* in virDomainDriverNodeDeviceReAttach() libxl_driver.c: validate 'driverName' earlier in libxlNodeDeviceDetachFlags() qemu_driver.c: validate 'driverName' earlier in qemuNodeDeviceDetachFlags() qemu, libxl, hypervisor: use virDomainDriverNodeDeviceDetachFlags() helper domain_driver.c: use g_auto* in virDomainDriverNodeDeviceDetachFlags()
src/datatypes.h | 2 + src/hypervisor/domain_driver.c | 147 +++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 11 +++ src/hypervisor/meson.build | 1 + src/libvirt_private.syms | 3 + src/libxl/libxl_driver.c | 164 +++------------------------------ src/qemu/qemu_driver.c | 164 ++++----------------------------- 7 files changed, 192 insertions(+), 300 deletions(-)
To what is in here: Reviewed-by: Ján Tomko <jtomko@redhat.com> But before this series is merged, a change to the list of files for checking ACL calls is needed. Jano
participants (3)
-
Daniel Henrique Barboza -
Daniel P. Berrangé -
Ján Tomko