* [PATCH] device-assignment: Better fd tracking
@ 2010-07-07 16:29 Alex Williamson
2010-07-07 17:28 ` Don Dutile
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Alex Williamson @ 2010-07-07 16:29 UTC (permalink / raw)
To: kvm; +Cc: ddutile, 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,
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] device-assignment: Better fd tracking
2010-07-07 16:29 [PATCH] device-assignment: Better fd tracking Alex Williamson
@ 2010-07-07 17:28 ` Don Dutile
2010-07-07 17:49 ` Alex Williamson
2010-07-08 8:32 ` Avi Kivity
2010-07-08 14:58 ` [PATCH v2] " Alex Williamson
2 siblings, 1 reply; 7+ messages in thread
From: Don Dutile @ 2010-07-07 17:28 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm
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 ?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] device-assignment: Better fd tracking
2010-07-07 17:28 ` Don Dutile
@ 2010-07-07 17:49 ` Alex Williamson
0 siblings, 0 replies; 7+ messages in thread
From: Alex Williamson @ 2010-07-07 17:49 UTC (permalink / raw)
To: Don Dutile; +Cc: kvm
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] device-assignment: Better fd tracking
2010-07-07 16:29 [PATCH] device-assignment: Better fd tracking Alex Williamson
2010-07-07 17:28 ` Don Dutile
@ 2010-07-08 8:32 ` Avi Kivity
2010-07-08 14:58 ` [PATCH v2] " Alex Williamson
2 siblings, 0 replies; 7+ messages in thread
From: Avi Kivity @ 2010-07-08 8:32 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, ddutile
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] device-assignment: Better fd tracking
2010-07-07 16:29 [PATCH] device-assignment: Better fd tracking Alex Williamson
2010-07-07 17:28 ` Don Dutile
2010-07-08 8:32 ` Avi Kivity
@ 2010-07-08 14:58 ` Alex Williamson
2010-07-08 15:28 ` Don Dutile
2010-07-12 18:12 ` Marcelo Tosatti
2 siblings, 2 replies; 7+ messages in thread
From: Alex Williamson @ 2010-07-08 14:58 UTC (permalink / raw)
To: kvm, avi; +Cc: alex.williamson, ddutile
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,
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] device-assignment: Better fd tracking
2010-07-08 14:58 ` [PATCH v2] " Alex Williamson
@ 2010-07-08 15:28 ` Don Dutile
2010-07-12 18:12 ` Marcelo Tosatti
1 sibling, 0 replies; 7+ messages in thread
From: Don Dutile @ 2010-07-08 15:28 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, avi
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] device-assignment: Better fd tracking
2010-07-08 14:58 ` [PATCH v2] " Alex Williamson
2010-07-08 15:28 ` Don Dutile
@ 2010-07-12 18:12 ` Marcelo Tosatti
1 sibling, 0 replies; 7+ messages in thread
From: Marcelo Tosatti @ 2010-07-12 18:12 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, avi, ddutile
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-07-12 19:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-07-07 16:29 [PATCH] device-assignment: Better fd tracking Alex Williamson
2010-07-07 17:28 ` Don Dutile
2010-07-07 17:49 ` Alex Williamson
2010-07-08 8:32 ` Avi Kivity
2010-07-08 14:58 ` [PATCH v2] " Alex Williamson
2010-07-08 15:28 ` Don Dutile
2010-07-12 18:12 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).