[PATCH] device-assignment: Better fd tracking

[PATCH] device-assignment: Better fd tracking

[Alex Williamson]

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,

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

[Don Dutile]

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 ?

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

[Alex Williamson]

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

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

[Avi Kivity]

On 07/07/2010 07:29 PM, 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.
>
> @@ -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);
>    

Missing  { }s.

>                   }
>   	    }
>           }
> @@ -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;
> -        }
>    

Again.

>
>   #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;
>       }
>    

-error is better than -1, but this is not introduces by this patch, so okay.

-- 
error compiling committee.c: too many arguments to function

[PATCH v2] device-assignment: Better fd tracking

[Alex Williamson]

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>
---

 v2: fix qemu style braces

 hw/device-assignment.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 48ac73c..de9470c 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,9 @@ 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,9 +796,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
@@ -1415,7 +1417,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,

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

[Don Dutile]

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>
> ---
> 
>  v2: fix qemu style braces
> 
>  hw/device-assignment.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 48ac73c..de9470c 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,9 @@ 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,9 +796,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
> @@ -1415,7 +1417,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,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

thanks for reply on other if check's.

Acked-by: Donald Dutile <ddutile@redhat.com>

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

[Marcelo Tosatti]

On Thu, Jul 08, 2010 at 08:58:32AM -0600, 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>
> ---
> 
>  v2: fix qemu style braces
> 
>  hw/device-assignment.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)

Applied, thanks.