Re: [libvirt] [PATCH v2] qemu: Unlink temporary file on failure

2011-08-02 Thread Eric Blake 

On 08/02/2011 09:50 AM, Michal Privoznik wrote:

Although virFDStreamOpenFile will unlink it once opened, when we hit
error path, we must unlink it by hand.
---
  src/qemu/qemu_driver.c |5 -
  1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 09b2791..c78cdb7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom,

  endjob:
  VIR_FORCE_CLOSE(tmp_fd);
-VIR_FREE(tmp);
+ if (tmp) {
+unlink(tmp);
+VIR_FREE(tmp);
+ }


ACK.

The VIR_FREE(tmp) could be unconditional, but it doesn't hurt to leave 
it conditional - at any rate, it won't trip the syntax-check for 
useless-if-before-free because we are doing more than just freeing tmp 
in the conditional.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemu: Unlink temporary file on failure

2011-08-02 Thread Eric Blake 

On 08/02/2011 09:59 AM, Eric Blake wrote:

On 08/02/2011 09:50 AM, Michal Privoznik wrote:

Although virFDStreamOpenFile will unlink it once opened, when we hit
error path, we must unlink it by hand.
---
src/qemu/qemu_driver.c | 5 -
1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 09b2791..c78cdb7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom,

endjob:
VIR_FORCE_CLOSE(tmp_fd);
- VIR_FREE(tmp);
+ if (tmp) {


Wonky spacing.


+ unlink(tmp);
+ VIR_FREE(tmp);
+ }


ACK.


I went ahead and pushed this after fixing the spacing, so that it will 
be in 0.9.4.


Meanwhile, I wonder if we have a bigger bug - that is, should 
virFDStreamOpenInternal guarantee that the file is deleted when 
requested, even on failure paths, so that callers need not do the 
unlink()?  That is, on the success path, we end up unlink()ing the same 
name twice, which is racy if the name is predictable (in the case of 
qemuDomainScreenshot, the name is temporary and supposedly safe).


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list