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

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 2010-07-07 at 13:28 -0400, Don Dutile wrote:
> 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 ?

This first check is only doing a sanity check, nothing gets setup, so
it's safe to just return directly.  get_real_device() has a couple paths
where it can exit after it has at least opened the config_fd, so it'd be
nice to close that down, which free_assigned_device() does.  I figure
free_assigned_device() is safe to call for any exit from
get_real_device() because a) setting the config_fd is the first thing it
does, so config_fd should always be set to something and b) the
resource_fds are still protected by the valid flag, which will be zero
because the structure is allocated with mallocz, so we won't try to free
uninitialized resource_fds.  Thanks,

Alex