Re: [libvirt] [PATCH] introduce VIR_CLOSE to be used rather than close()

On 10/15/2010 12:54 PM, Matthias Bolte wrote: > 2010/10/15 Stefan Berger&lt;stefanb(a)linux.vnet.ibm.com&gt;: >> Since bugs due to double-closed filedescriptors are difficult to >> track down >> in a multi-threaded system, I am introducing the VIR_CLOSE(fd) macro >> to help >> avoid mistakes here. >> >> There are lots of places where close() is being used. In this patch >> I am >> only cleaning up usage of close() in src/conf where the problems were. >> >> I also dare to declare close() as being deprecated in libvirt code base >> (HACKING). :-) >> >> Signed-off-by: Stefan Berger&lt;stefanb(a)us.ibm.com&gt; > >> Index: libvirt-acl/src/conf/domain_conf.c >> =================================================================== >> --- libvirt-acl.orig/src/conf/domain_conf.c >> +++ libvirt-acl/src/conf/domain_conf.c >> @@ -46,6 +46,7 @@ >> #include "nwfilter_conf.h" >> #include "ignore-value.h" >> #include "storage_file.h" >> +#include "files.h" >> >> #define VIR_FROM_THIS VIR_FROM_DOMAIN >> >> @@ -6798,17 +6799,18 @@ int virDomainSaveXML(const char *configD >> goto cleanup; >> } >> >> - if (close(fd)< 0) { >> + if (VIR_CLOSE(fd)< 0) { >> virReportSystemError(errno, >> _("cannot save config file '%s'"), >> configFile); >> - goto cleanup; >> + goto cleanup_free; >> } >> >> ret = 0; >> cleanup: >> - if (fd != -1) >> - close(fd); >> + VIR_CLOSE(fd); >> + >> + cleanup_free: >> VIR_FREE(configFile); >> return ret; >> } > Why did you add a new label here? You build VIR_CLOSE in a way that > allows safe double invocation. Therefore, this is just fine, isn't it: I had it without this new label and then changed it since I think it's still good practice to not try to invoke the function twice, even though nothing can go wrong. Stefan