From mboxrd@z Thu Jan  1 00:00:00 1970
From: Don Dutile <ddutile@redhat.com>
Subject: Re: [PATCH] device-assignment: Better fd tracking
Date: Wed, 07 Jul 2010 13:28:09 -0400
Message-ID: <4C34B929.1080405@redhat.com>
References: <20100707162948.5414.51749.stgit@localhost.localdomain>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org
To: Alex Williamson <alex.williamson@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:31732 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754615Ab0GGR06 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 7 Jul 2010 13:26:58 -0400
Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o67HQvRx005467
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK)
	for <kvm@vger.kernel.org>; Wed, 7 Jul 2010 13:26:57 -0400
In-Reply-To: <20100707162948.5414.51749.stgit@localhost.localdomain>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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 ?