Re: [PATCH] device-assignment: Better fd tracking

[Don Dutile <ddutile@redhat.com>]

Alex Williamson wrote:
> Commit 909bfdba fixed a hole with not closing resource file descriptors
> but we need to be more careful about tracking which are real fds,
> otherwise we might close fd 0, which doesn't work out so well for stdio.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  hw/device-assignment.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 48ac73c..3bcb63d 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -694,6 +694,7 @@ again:
>  
>          rp = dev->regions + r;
>          rp->valid = 0;
> +        rp->resource_fd = -1;
>          size = end - start + 1;
>          flags &= IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH;
>          if (size == 0 || (flags & ~IORESOURCE_PREFETCH) == 0)
> @@ -785,7 +786,8 @@ static void free_assigned_device(AssignedDevice *dev)
>                          fprintf(stderr,
>  				"Failed to unmap assigned device region: %s\n",
>  				strerror(errno));
> -                    close(pci_region->resource_fd);
> +                    if (pci_region->resource_fd >= 0)
> +                        close(pci_region->resource_fd);
>                  }
>  	    }
>          }
> @@ -793,10 +795,8 @@ static void free_assigned_device(AssignedDevice *dev)
>          if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX)
>              assigned_dev_unregister_msix_mmio(dev);
>  
> -        if (dev->real_device.config_fd) {
> +        if (dev->real_device.config_fd >= 0)
>              close(dev->real_device.config_fd);
> -            dev->real_device.config_fd = 0;
> -        }
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>          free_dev_irq_entries(dev);
> @@ -1415,7 +1415,7 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
>  
>      if (!dev->host.seg && !dev->host.bus && !dev->host.dev && !dev->host.func) {
>          error_report("pci-assign: error: no host device specified");
> -        goto out;
> +        return -1;
>      }
>  
>      if (get_real_device(dev, dev->host.seg, dev->host.bus,
> 

and why not change the goto out   in the next 2 if-check's to return -1
to skip the free_assigned_device(dev) call for those 2 cases as well ?