[libvirt] [PATCH v1 03/31] virpcimock: Move actions checking one level up

[Peter Krempa]

On Thu, Jul 11, 2019 at 17:53:50 +0200, Michal Privoznik wrote:
> The pci_driver_bind() and pci_driver_unbind() functions are
> "internal implementation", meaning other parts of the code should
> be able to call them and get the job done. Checking for actions
> (PCI_ACTION_BIND and PCI_ACTION_UNBIND) should be done in
> handlers (pci_driver_handle_bind() and
> pci_driver_handle_unbind()). Surprisingly, the other two actions
> (PCI_ACTION_NEW_ID and PCI_ACTION_REMOVE_ID) are checked already
> at this level.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  tests/virpcimock.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
> index beb5e1490d..6865f992dc 100644
> --- a/tests/virpcimock.c
> +++ b/tests/virpcimock.c
> @@ -551,8 +551,8 @@ pci_driver_bind(struct pciDriver *driver,
>      int ret = -1;
>      char *devpath = NULL, *driverpath = NULL;
>  
> -    if (dev->driver || PCI_ACTION_BIND & driver->fail) {
> -        /* Device already bound or failing driver requested */
> +    if (dev->driver) {
> +        /* Device already bound */
>          errno = ENODEV;
>          return ret;
>      }

So this function ...

> @@ -669,8 +669,8 @@ pci_driver_handle_bind(const char *path)
>      struct pciDevice *dev = pci_device_find_by_content(path);
>      struct pciDriver *driver = pci_driver_find_by_path(path);
>  
> -    if (!driver || !dev) {
> -        /* This should never happen (TM) */
> +    if (!driver || !dev || PCI_ACTION_BIND & driver->fail) {
> +        /* No driver, no device or failing driver requested */
>          errno = ENODEV;
>          goto cleanup;
>      }

... is called here, which you fix, but also in

pci_device_autobind and pci_driver_handle_new_id which are not fixed by
this commit.

I don't quite understand deeply what this is supposed to do, so I don't
know what's supposed to happen in that case, but this seems suspicious
to me. Please try explaining/justifying why the two other call paths are
not changed.

Also I did not bother checking the unbind code for the same problem.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190716/908a846b/attachment-0001.sig>