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@xxxxxxxxxx>
> > ---
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
