Re: [libvirt] [PATCH v2 3/9] hostdev: Use common reattach code in virHostdevPCINodeDeviceReAttach()
On 01/27/2016 12:39 PM, Andrea Bolognani wrote: > On Tue, 2016-01-26 at 18:56 -0500, John Ferlan wrote: >>> +/** >>> + * virHostdevPCINodeDeviceReAttach: >> >> ^^ Oy ReAttach vs. Reattach is an eye test ;-) > > Maybe we should standardize on either one or the other? I personally > consider "ReAttach" to be quite an eyesore, but then again it's all > over the public API so it's not going anywhere... > Sadly, I think we're stuck with that CaMel case because it's a driver function... >>> + * @hostdev_mgr: hostdev manager >>> + * @pci: PCI device >> >> Perhaps better to indicate a "new"ly generated PCI device that does not >> track the internal reattach states and other state information such as >> the stub driver. >> >> IOW: this is not a copy of an [in]activePCIHostdevs element > > Great idea! Maybe even use a different name for the parameter, based > on whether the virPCIDevicePtr is going to be used for something other > than looking up the actual device? This is I believe where I went back to patch 1 and started thinking about what is passed in 'pci'... Anything to help make things more obvious could be beneficial, especially considering what this code ends up doing... John > > Cheers. > > -- > Andrea Bolognani > Software Engineer - Virtualization Team > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/9] hostdev: Use common reattach code in virHostdevPCINodeDeviceReAttach()
On Tue, 2016-01-26 at 18:56 -0500, John Ferlan wrote: > > +/** > > + * virHostdevPCINodeDeviceReAttach: > > ^^ Oy ReAttach vs. Reattach is an eye test ;-) Maybe we should standardize on either one or the other? I personally consider "ReAttach" to be quite an eyesore, but then again it's all over the public API so it's not going anywhere... > > + * @hostdev_mgr: hostdev manager > > + * @pci: PCI device > > Perhaps better to indicate a "new"ly generated PCI device that does not > track the internal reattach states and other state information such as > the stub driver. > > IOW: this is not a copy of an [in]activePCIHostdevs element Great idea! Maybe even use a different name for the parameter, based on whether the virPCIDevicePtr is going to be used for something other than looking up the actual device? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/9] hostdev: Use common reattach code in virHostdevPCINodeDeviceReAttach()
On 01/25/2016 11:20 AM, Andrea Bolognani wrote:
> This ensures the behavior for reattach is consistent, no matter how it
> was triggered (eg. 'virsh nodedev-reattach', 'virsh detach-device' or
> shutdown of a domain that is configured to use hostdevs).
Similar to 2/9 - using virHostdevOnlyReattachPCIDevice will result in a
call to virPCIDeviceGetStubDriver and virPCIDeviceWaitForCleanup - for
this path I assume this is thus desired.
> ---
> src/util/virhostdev.c | 33 ++---
> 1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 586937e..f40d636 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -1637,10 +1637,24 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr
> hostdev_mgr,
> return ret;
> }
>
> +/**
> + * virHostdevPCINodeDeviceReAttach:
^^ Oy ReAttach vs. Reattach is an eye test ;-)
> + * @hostdev_mgr: hostdev manager
> + * @pci: PCI device
Perhaps better to indicate a "new"ly generated PCI device that does not
track the internal reattach states and other state information such as
the stub driver.
IOW: this is not a copy of an [in]activePCIHostdevs element
> + *
> + * Reattach a specific PCI device to the host.
> + *
> + * This function makes sure the device is not in use before reattaching it
> + * to the host; if the device has already been reattached to the host, the
> + * operation is considered successful.
> + *
> + * Returns: 0 on success, <0 on failure
> + */
> int
> virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr,
> virPCIDevicePtr pci)
> {
> +virPCIDevicePtr actual;
> struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL,
> false};
> int ret = -1;
> @@ -1651,10 +1665,23 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr
> hostdev_mgr,
> if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), ))
> goto out;
>
> -virPCIDeviceReattachInit(pci);
> +/* We want this function to be idempotent, so if the device has already
> + * been removed from the inactive list (and is not active either, as
> + * per the check above) just return right away. We also need to retrieve
> + * the actual device from the inactive list because that's the one that
> + * contains state information such as the stub driver */
> +if (!(actual = virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs,
> +pci))) {
As noted in my 1/9 review - this code path passes the 'actual' dev
onlist... The call we're about to make is going to repeat this search.
Something we could avoid if performance of list searches were a concern.
No other issues -
John
> +VIR_DEBUG("PCI device %s is already attached to the host",
> + virPCIDeviceGetName(pci));
> +ret = 0;
> +goto out;
> +}
>
> -if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs,
> - hostdev_mgr->inactivePCIHostdevs) < 0)
> +/* Reattach the device. We don't want to skip unmanaged devices in
> + * this case, because the user explicitly asked for the device to
> + * be reattached to the host driver */
> +if (virHostdevOnlyReattachPCIDevice(hostdev_mgr, actual, false) < 0)
> goto out;
>
> ret = 0;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/9] hostdev: Use common reattach code in virHostdevPCINodeDeviceReAttach()
This ensures the behavior for reattach is consistent, no matter how it
was triggered (eg. 'virsh nodedev-reattach', 'virsh detach-device' or
shutdown of a domain that is configured to use hostdevs).
---
src/util/virhostdev.c | 33 ++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 586937e..f40d636 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -1637,10 +1637,24 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr
hostdev_mgr,
return ret;
}
+/**
+ * virHostdevPCINodeDeviceReAttach:
+ * @hostdev_mgr: hostdev manager
+ * @pci: PCI device
+ *
+ * Reattach a specific PCI device to the host.
+ *
+ * This function makes sure the device is not in use before reattaching it
+ * to the host; if the device has already been reattached to the host, the
+ * operation is considered successful.
+ *
+ * Returns: 0 on success, <0 on failure
+ */
int
virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr,
virPCIDevicePtr pci)
{
+virPCIDevicePtr actual;
struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL,
false};
int ret = -1;
@@ -1651,10 +1665,23 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr
hostdev_mgr,
if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), ))
goto out;
-virPCIDeviceReattachInit(pci);
+/* We want this function to be idempotent, so if the device has already
+ * been removed from the inactive list (and is not active either, as
+ * per the check above) just return right away. We also need to retrieve
+ * the actual device from the inactive list because that's the one that
+ * contains state information such as the stub driver */
+if (!(actual = virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs,
+pci))) {
+VIR_DEBUG("PCI device %s is already attached to the host",
+ virPCIDeviceGetName(pci));
+ret = 0;
+goto out;
+}
-if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs,
- hostdev_mgr->inactivePCIHostdevs) < 0)
+/* Reattach the device. We don't want to skip unmanaged devices in
+ * this case, because the user explicitly asked for the device to
+ * be reattached to the host driver */
+if (virHostdevOnlyReattachPCIDevice(hostdev_mgr, actual, false) < 0)
goto out;
ret = 0;
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
