On 10/15/2010 01:01 PM, Stefan Berger wrote:
On 10/15/2010 12:54 PM, Matthias Bolte wrote:
2010/10/15 Stefan Berger<stefanb@xxxxxxxxxxxxxxxxxx>:
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<stefanb@xxxxxxxxxx>
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
VIR_FREE() is commonly used in the way that Matthias describes, so I
think it would be consistent to use VIR_CLOSE in this way as well.
Invoking it unconditionally simplifies the code, and eliminates possible
confusion when someone adds new code and isn't sure which label they
should goto on failure (or maybe they think they are sure, but make the
wrong assumption, thus leading to a leaked fd as a result of trying to
eliminate possible double-closed fd's ;-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list