Re: [PATCH] bye to close(), welcome to VIR_(FORCE_)CLOSE()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/22/2010 05:19 AM, Stefan Berger wrote:
Using automated replacement with sed and editing I have now replaced all
occurrences of close() with VIR_(FORCE_)CLOSE() except for one, of
course. Some replacements were straight forward, others I needed to pay
attention. I hope I payed attention in all the right places... Please
have a look. This should have at least solved one more double-close
error.

Signed-off-by: Stefan Berger<stefanb@xxxxxxxxxx>



Index: libvirt-acl/src/libvirt.c
===================================================================
--- libvirt-acl.orig/src/libvirt.c
+++ libvirt-acl/src/libvirt.c
@@ -10794,7 +10794,7 @@ virStreamRef(virStreamPtr stream)
   *      ... report an error ....
   * done:
   *   virStreamFree(st);
- *   close(fd);
+ *   VIR_FORCE_CLOSE(fd);

Ignoring close() failure on read seems reasonable...

   *
   * Returns the number of bytes written, which may be less
   * than requested.
@@ -10884,7 +10884,7 @@ error:
   *      ... report an error ....
   * done:
   *   virStreamFree(st);
- *   close(fd);
+ *   VIR_FORCE_CLOSE(fd);

but on write, we should instead change the recommendation to check for close() failure, since some file systems (yes, I'm looking at you, NFS) can successfully write() to kernel buffers but still fail to close() when actual network traffic is finally triggered.

 *   int fd = open("demo.iso", O_WRONLY, 0600)
 *
...
 *   if (virStreamFinish(st) < 0 || VIR_CLOSE(fd) < 0)
 *      ... report an error ....
 * done:
 *   virStreamFree(st);
 *   VIR_FORCE_CLOSE(fd);

I'm wondering how much of the rest of your patch might be impacted by adopting this idiom.

Also, should we be thinking of a separate patch for VIR_FCLOSE for the FILE* variant?

--
Eric Blake   eblake@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]