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: > 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
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
>
© 2016 - 2026 Red Hat, Inc.