Next patch will use g_autoptr() with virNodeDevicePtr for cleanups. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- 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

libxlNodeDeviceReAttach() and qemuNodeDeviceReAttach() are mostly equal, differing only how the virHostdevManager pointer is retrieved. Put the common code into virDomainDriverNodeDeviceReAttach() to reduce code duplication. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- 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 814a6c282c..316a6c6bf5 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

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. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- 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 316a6c6bf5..eadacdfb76 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

On 2/2/21 8:06 PM, Daniel Henrique Barboza wrote: Coverity points out the rest of this is unreachable now (even with the subsequent patch). This section seems to be more than just code motion... it used to pass thru here when vfio = true, but with the movement it would fall into the catch all else. John

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. The helper is named with the 'Flags' suffix, even when the helper itself isn't receiving the flags from the callers, to be compliant with the ACL function virNodeDeviceDetachFlagsEnsureACL() that is being called inside it and was called from the original functions. Renaming the helper would implicate in renaming REMOTE_PROC_NODE_DEVICE_DETACH_FLAGS, and all the related structs inside remote_protocol.x, to be compliant with the ACL rules. This is not being checked at this moment, but we'll fix check-aclrules.py to verify all the helpers that calls ACL functions in domain_driver.c shortly. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- 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 eadacdfb76..a644593dd9 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

This script works under two specific conditions. For each opened file, search for all functions that has ACL calls and store them, and see if there is a vir*DriverPtr struct declared in it. For each implementation found, check if there is an ACL verification inside it, and error out if none was found. The script also supports the concept of stub, where another function takes the responsibility for the ACL call instead of the original API. Unfortunately this is not enough to cover the new scenario we have now, with domain_driver.c containing helper functions that execute the ACL calls. The script does not store state between files because, until now, it wasn't needed to - APIs and stubs and vir*DriverPtr declarations were always in the same file. Also, the script will not check for ACL in functions that does not belong to a vir*DriverPtr interface. What we have now in domain_driver.c breaks both assumptions: the functions are in a different file, and there is no vir*DriverPtr being implemented in the file that uses these functions. This patch changes check-aclrules.py to accomodate this scenario. The helpers that have ACL checks are stored beforehand in aclFuncHelpers, allowing other files to use them to recognize a stub situation. In case the current file being analyzed is domain_driver.c itself, we'll do a manual check using aclFuncHelpers to verify that these functions indeed have ACL checks. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- scripts/check-aclrules.py | 25 ++++++++++++++++++++++++- src/hypervisor/meson.build | 2 ++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/scripts/check-aclrules.py b/scripts/check-aclrules.py index 2335e8cfdd..ed6805058b 100755 --- a/scripts/check-aclrules.py +++ b/scripts/check-aclrules.py @@ -62,6 +62,14 @@ implpermitted = { "vzDomainMigrateConfirm3Params": True, } +aclFuncHelpers = { + "virDomainDriverNodeDeviceDetachFlags": True, + "virDomainDriverNodeDeviceReset": True, + "virDomainDriverNodeDeviceReAttach": True, +} + +aclFuncHelperFile = "domain_driver.c" + lastfile = None @@ -136,8 +144,14 @@ def process_file(filename): maybefunc = None intable = False table = None + aclHelperFileCheck = False + + acls = aclFuncHelpers + + if aclFuncHelperFile in filename: + acls = {} + aclHelperFileCheck = True - acls = {} aclfilters = {} errs = False with open(filename, "r") as fh: @@ -262,6 +276,15 @@ def process_file(filename): if "}" in line: brace = brace - 1 + if aclHelperFileCheck: + for helper in aclFuncHelpers: + if helper not in acls: + print(("%s:%d Missing ACL check in helper function '%s'") % + (filename, lineno, helper), + file=sys.stderr) + + errs = True + return errs diff --git a/src/hypervisor/meson.build b/src/hypervisor/meson.build index 32d5ab365f..70801c0820 100644 --- a/src/hypervisor/meson.build +++ b/src/hypervisor/meson.build @@ -5,6 +5,8 @@ hypervisor_sources = [ 'virhostdev.c', ] +stateful_driver_source_files += files(hypervisor_sources) + hypervisor_lib = static_library( 'virt_hypervisor', [ -- 2.26.2