V3: - fixes from V2 review + one lost hunk -> diff(tree+V2, tree+v3) at bottom of email V2: - following Eric's suggestion, deprecating fdclose(), introducing VIR_FDCLOSE() - following some other nits that Eric pointed out - replaced some more fclose () I previously had missed (last 2 files in patch) Similarly to deprecating close(), I am now deprecating fclose() and introduce VIR_FORCE_FCLOSE() and VIR_FCLOSE(). Also, fdopen() is replaced with VIR_FDOPEN(). Most of the files are opened in read-only mode, so usage of VIR_FORCE_CLOSE() seemed appropriate. Others that are opened in write mode already had the fclose()< 0 check and I converted those to VIR_FCLOSE()< 0. I did not find occurrences of possible double-closed files on the way. Signed-off-by: Stefan Berger&lt;stefanb(a)us.ibm.com&gt; --- HACKING | 33 +++++++++++++++++++++++++++------ daemon/libvirtd.c | 6 +++--- docs/hacking.html.in | 36 ++++++++++++++++++++++++++++-------- src/libvirt_private.syms | 2 ++ src/nodeinfo.c | 8 ++++---- src/openvz/openvz_conf.c | 16 +++++++++------- src/openvz/openvz_driver.c | 4 ++-- src/qemu/qemu_driver.c | 4 ++-- src/storage/storage_backend.c | 18 +++++++----------- src/storage/storage_backend_fs.c | 4 ++-- src/storage/storage_backend_iscsi.c | 9 +++------ src/storage/storage_backend_scsi.c | 2 +- src/uml/uml_driver.c | 8 ++++---- src/util/cgroup.c | 10 +++++----- src/util/dnsmasq.c | 5 +++-- src/util/files.c | 34 ++++++++++++++++++++++++++++++++++ src/util/files.h | 10 +++++++++- src/util/macvtap.c | 4 ++-- src/util/stats_linux.c | 5 +++-- src/util/util.c | 10 ++++------ src/xen/block_stats.c | 3 ++- src/xen/xen_driver.c | 7 ++++--- src/xen/xen_hypervisor.c | 8 +++----- tests/nodeinfotest.c | 5 +++-- tests/testutils.c | 8 ++++---- tests/xencapstest.c | 7 +++---- 26 files changed, 173 insertions(+), 93 deletions(-) Index: libvirt-acl/src/util/files.c =================================================================== --- libvirt-acl.orig/src/util/files.c +++ libvirt-acl/src/util/files.c @@ -44,3 +44,37 @@ int virClose(int *fdptr, bool preserve_e return rc; } + + +int virFclose(FILE **file, bool preserve_errno) +{ + int saved_errno; + int rc = 0; + + if (*file) { + if (preserve_errno) + saved_errno = errno; + rc = fclose(*file); + *file = NULL; + if (preserve_errno) + errno = saved_errno; + } + + return rc; +} + + +FILE *virFdopen(int *fdptr, const char *mode) +{ + FILE *file = NULL; + + if (*fdptr>= 0) { + file = fdopen(*fdptr, mode); + if (file) + *fdptr = -1; + } else { + errno = EBADF; + } + + return file; +} Index: libvirt-acl/src/util/files.h =================================================================== --- libvirt-acl.orig/src/util/files.h +++ libvirt-acl/src/util/files.h @@ -27,20 +27,28 @@ # define __VIR_FILES_H_ # include<stdbool.h> +# include<stdio.h> # include "internal.h" # include "ignore-value.h" -/* Don't call this directly - use the macros below */ +/* Don't call these directly - use the macros below */ int virClose(int *fdptr, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; +int virFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; +FILE *virFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; /* For use on normal paths; caller must check return value, and failure sets errno per close(). */ # define VIR_CLOSE(FD) virClose(&(FD), false) +# define VIR_FCLOSE(FILE) virFclose(&(FILE), false) + +/* Wrapper around fdopen that consumes fd on success. */ +# define VIR_FDOPEN(FD, MODE) virFdopen(&(FD), MODE) /* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ # define VIR_FORCE_CLOSE(FD) ignore_value(virClose(&(FD), true)) +# define VIR_FORCE_FCLOSE(FILE) ignore_value(virFclose(&(FILE), true)) #endif /* __VIR_FILES_H */ Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -344,6 +344,8 @@ virFDStreamCreateFile; # files.h virClose; +virFclose; +virFdopen; # hash.h Index: libvirt-acl/daemon/libvirtd.c =================================================================== --- libvirt-acl.orig/daemon/libvirtd.c +++ libvirt-acl/daemon/libvirtd.c @@ -512,7 +512,7 @@ static int qemudWritePidFile(const char return -1; } - if (!(fh = fdopen(fd, "w"))) { + if (!(fh = VIR_FDOPEN(fd, "w"))) { VIR_ERROR(_("Failed to fdopen pid file '%s' : %s"), pidFile, virStrerror(errno, ebuf, sizeof ebuf)); VIR_FORCE_CLOSE(fd); @@ -522,11 +522,11 @@ static int qemudWritePidFile(const char if (fprintf(fh, "%lu\n", (unsigned long)getpid())< 0) { VIR_ERROR(_("%s: Failed to write to pid file '%s' : %s"), argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf)); - fclose(fh); + VIR_FORCE_FCLOSE(fh); return -1; } - if (fclose(fh) == EOF) { + if (VIR_FCLOSE(fh) == EOF) { VIR_ERROR(_("%s: Failed to close pid file '%s' : %s"), argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf)); return -1; Index: libvirt-acl/src/nodeinfo.c =================================================================== --- libvirt-acl.orig/src/nodeinfo.c +++ libvirt-acl/src/nodeinfo.c @@ -46,6 +46,7 @@ #include "virterror_internal.h" #include "count-one-bits.h" #include "intprops.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -102,8 +103,7 @@ get_cpu_value(unsigned int cpu, const ch } cleanup: - if (pathfp) - fclose(pathfp); + VIR_FORCE_FCLOSE(pathfp); VIR_FREE(path); return value; @@ -155,7 +155,7 @@ static unsigned long count_thread_siblin } cleanup: - fclose(pathfp); + VIR_FORCE_FCLOSE(pathfp); VIR_FREE(path); return ret; @@ -329,7 +329,7 @@ int nodeGetInfo(virConnectPtr conn ATTRI return -1; } ret = linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo); - fclose(cpuinfo); + VIR_FORCE_FCLOSE(cpuinfo); if (ret< 0) return -1; Index: libvirt-acl/src/openvz/openvz_conf.c =================================================================== --- libvirt-acl.orig/src/openvz/openvz_conf.c +++ libvirt-acl/src/openvz/openvz_conf.c @@ -528,7 +528,7 @@ int openvzLoadDomains(struct openvz_driv dom = NULL; } - fclose(fp); + VIR_FORCE_FCLOSE(fp); return 0; @@ -536,7 +536,7 @@ int openvzLoadDomains(struct openvz_driv virReportOOMError(); cleanup: - fclose(fp); + VIR_FORCE_FCLOSE(fp); if (dom) virDomainObjUnref(dom); return -1; @@ -888,6 +888,7 @@ openvzSetDefinedUUID(int vpsid, unsigned { char *conf_file; char uuidstr[VIR_UUID_STRING_BUFLEN]; + FILE *fp = NULL; int ret = -1; if (uuid == NULL) @@ -900,21 +901,22 @@ openvzSetDefinedUUID(int vpsid, unsigned goto cleanup; if (uuidstr[0] == 0) { - FILE *fp = fopen(conf_file, "a"); /* append */ + fp = fopen(conf_file, "a"); /* append */ if (fp == NULL) goto cleanup; virUUIDFormat(uuid, uuidstr); - /* Record failure if fprintf or fclose fails, + /* Record failure if fprintf or VIR_FCLOSE fails, and be careful always to close the stream. */ - if ((fprintf(fp, "\n#UUID: %s\n", uuidstr)< 0) - + (fclose(fp) == EOF)) + if ((fprintf(fp, "\n#UUID: %s\n", uuidstr)< 0) || + (VIR_FCLOSE(fp) == EOF)) goto cleanup; } ret = 0; cleanup: + VIR_FORCE_FCLOSE(fp); VIR_FREE(conf_file); return ret; } @@ -996,7 +998,7 @@ int openvzGetVEID(const char *name) { } ok = fscanf(fp, "%d\n",&veid ) == 1; - fclose(fp); + VIR_FORCE_FCLOSE(fp); if (ok&& veid>= 0) return veid; Index: libvirt-acl/src/openvz/openvz_driver.c =================================================================== --- libvirt-acl.orig/src/openvz/openvz_driver.c +++ libvirt-acl/src/openvz/openvz_driver.c @@ -154,7 +154,7 @@ openvzDomainDefineCmd(const char *args[] max_veid = veid; } } - fclose(fp); + VIR_FORCE_FCLOSE(fp); if (max_veid == 0) { max_veid = 100; @@ -189,7 +189,7 @@ no_memory: return -1; cleanup: - fclose(fp); + VIR_FORCE_FCLOSE(fp); return -1; #undef ADD_ARG Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -4588,7 +4588,7 @@ static int qemudGetProcessInfo(unsigned /* startstack -> processor */ "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d", &usertime,&systime,&cpu) != 3) { - fclose(pidinfo); + VIR_FORCE_FCLOSE(pidinfo); VIR_WARN0("cannot parse process status data"); errno = -EINVAL; return -1; @@ -4608,7 +4608,7 @@ static int qemudGetProcessInfo(unsigned VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d", pid, tid, usertime, systime, cpu); - fclose(pidinfo); + VIR_FORCE_FCLOSE(pidinfo); return 0; } Index: libvirt-acl/src/storage/storage_backend.c =================================================================== --- libvirt-acl.orig/src/storage/storage_backend.c +++ libvirt-acl/src/storage/storage_backend.c @@ -1398,7 +1398,7 @@ virStorageBackendRunProgRegex(virStorage goto cleanup; } - if ((list = fdopen(fd, "r")) == NULL) { + if ((list = VIR_FDOPEN(fd, "r")) == NULL) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot read fd")); goto cleanup; @@ -1458,10 +1458,8 @@ virStorageBackendRunProgRegex(virStorage VIR_FREE(reg); - if (list) - fclose(list); - else - VIR_FORCE_CLOSE(fd); + VIR_FORCE_FCLOSE(list); + VIR_FORCE_CLOSE(fd); while ((err = waitpid(child,&exitstatus, 0) == -1)&& errno == EINTR); @@ -1531,9 +1529,9 @@ virStorageBackendRunProgNul(virStoragePo goto cleanup; } - if ((fp = fdopen(fd, "r")) == NULL) { + if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot read fd")); + "%s", _("cannot open file using fd")); goto cleanup; } @@ -1573,10 +1571,8 @@ virStorageBackendRunProgNul(virStoragePo VIR_FREE(v[i]); VIR_FREE(v); - if (fp) - fclose (fp); - else - VIR_FORCE_CLOSE(fd); + VIR_FORCE_FCLOSE(fp); + VIR_FORCE_CLOSE(fd); while ((w_err = waitpid (child,&exitstatus, 0) == -1)&& errno == EINTR) /* empty */ ; Index: libvirt-acl/src/storage/storage_backend_fs.c =================================================================== --- libvirt-acl.orig/src/storage/storage_backend_fs.c +++ libvirt-acl/src/storage/storage_backend_fs.c @@ -284,12 +284,12 @@ virStorageBackendFileSystemIsMounted(vir while ((getmntent_r(mtab,&ent, buf, sizeof(buf))) != NULL) { if (STREQ(ent.mnt_dir, pool->def->target.path)) { - fclose(mtab); + VIR_FORCE_FCLOSE(mtab); return 1; } } - fclose(mtab); + VIR_FORCE_FCLOSE(mtab); return 0; } Index: libvirt-acl/src/storage/storage_backend_iscsi.c =================================================================== --- libvirt-acl.orig/src/storage/storage_backend_iscsi.c +++ libvirt-acl/src/storage/storage_backend_iscsi.c @@ -188,7 +188,7 @@ virStorageBackendIQNFound(virStoragePool goto out; } - if ((fp = fdopen(fd, "r")) == NULL) { + if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open stream for file descriptor " "when reading output from '%s': '%s'"), @@ -235,11 +235,8 @@ out: } VIR_FREE(line); - if (fp != NULL) { - fclose(fp); - } else { - VIR_FORCE_CLOSE(fd); - } + VIR_FORCE_FCLOSE(fp); + VIR_FORCE_CLOSE(fd); return ret; } Index: libvirt-acl/src/storage/storage_backend_scsi.c =================================================================== --- libvirt-acl.orig/src/storage/storage_backend_scsi.c +++ libvirt-acl/src/storage/storage_backend_scsi.c @@ -70,7 +70,7 @@ getDeviceType(uint32_t host, } gottype = fgets(typestr, 3, typefile); - fclose(typefile); + VIR_FORCE_FCLOSE(typefile); if (gottype == NULL) { virReportSystemError(errno, Index: libvirt-acl/src/uml/uml_driver.c =================================================================== --- libvirt-acl.orig/src/uml/uml_driver.c +++ libvirt-acl/src/uml/uml_driver.c @@ -587,11 +587,11 @@ reopen: if (fscanf(file, "%d",&vm->pid) != 1) { errno = EINVAL; - fclose(file); + VIR_FORCE_FCLOSE(file); goto cleanup; } - if (fclose(file)< 0) + if (VIR_FCLOSE(file)< 0) goto cleanup; rc = 0; @@ -1096,7 +1096,7 @@ static int umlGetProcessInfo(unsigned lo if (fscanf(pidinfo, "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu",&usertime,&systime) != 2) { umlDebug("not enough arg"); - fclose(pidinfo); + VIR_FORCE_FCLOSE(pidinfo); return -1; } @@ -1109,7 +1109,7 @@ static int umlGetProcessInfo(unsigned lo umlDebug("Got %llu %llu %llu", usertime, systime, *cpuTime); - fclose(pidinfo); + VIR_FORCE_FCLOSE(pidinfo); return 0; } Index: libvirt-acl/src/util/cgroup.c =================================================================== --- libvirt-acl.orig/src/util/cgroup.c +++ libvirt-acl/src/util/cgroup.c @@ -31,6 +31,7 @@ #include "memory.h" #include "cgroup.h" #include "logging.h" +#include "files.h" #define CGROUP_MAX_VAL 512 @@ -127,13 +128,12 @@ static int virCgroupDetectMounts(virCgro } } - fclose(mounts); + VIR_FORCE_FCLOSE(mounts); return 0; no_memory: - if (mounts) - fclose(mounts); + VIR_FORCE_FCLOSE(mounts); return -ENOMEM; } @@ -192,12 +192,12 @@ static int virCgroupDetectPlacement(virC } } - fclose(mapping); + VIR_FORCE_FCLOSE(mapping); return 0; no_memory: - fclose(mapping); + VIR_FORCE_FCLOSE(mapping); return -ENOMEM; } Index: libvirt-acl/src/util/dnsmasq.c =================================================================== --- libvirt-acl.orig/src/util/dnsmasq.c +++ libvirt-acl/src/util/dnsmasq.c @@ -44,6 +44,7 @@ #include "memory.h" #include "virterror_internal.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_NETWORK #define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile" @@ -171,7 +172,7 @@ hostsfileWrite(const char *path, for (i = 0; i< nhosts; i++) { if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { rc = errno; - fclose(f); + VIR_FORCE_FCLOSE(f); if (istmp) unlink(tmp); @@ -180,7 +181,7 @@ hostsfileWrite(const char *path, } } - if (fclose(f) == EOF) { + if (VIR_FCLOSE(f) == EOF) { rc = errno; goto cleanup; } Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -414,11 +414,11 @@ int openTap(const char *ifname, virReportSystemError(errno, "%s",_("cannot determine macvtap's tap device " "interface index")); - fclose(file); + VIR_FORCE_FCLOSE(file); return -1; } - fclose(file); + VIR_FORCE_FCLOSE(file); if (snprintf(tapname, sizeof(tapname), "/dev/tap%d", ifindex)>= sizeof(tapname)) { Index: libvirt-acl/src/util/util.c =================================================================== --- libvirt-acl.orig/src/util/util.c +++ libvirt-acl/src/util/util.c @@ -1816,7 +1816,7 @@ int virFileWritePidPath(const char *pidf goto cleanup; } - if (!(file = fdopen(fd, "w"))) { + if (!(file = VIR_FDOPEN(fd, "w"))) { rc = errno; VIR_FORCE_CLOSE(fd); goto cleanup; @@ -1830,10 +1830,8 @@ int virFileWritePidPath(const char *pidf rc = 0; cleanup: - if (file&& - fclose(file)< 0) { + if (VIR_FCLOSE(file)< 0) rc = errno; - } return rc; } @@ -1864,11 +1862,11 @@ int virFileReadPid(const char *dir, if (fscanf(file, "%d", pid) != 1) { rc = EINVAL; - fclose(file); + VIR_FORCE_FCLOSE(file); goto cleanup; } - if (fclose(file)< 0) { + if (VIR_FCLOSE(file)< 0) { rc = errno; goto cleanup; } Index: libvirt-acl/src/xen/xen_driver.c =================================================================== --- libvirt-acl.orig/src/xen/xen_driver.c +++ libvirt-acl/src/xen/xen_driver.c @@ -47,6 +47,7 @@ #include "pci.h" #include "uuid.h" #include "fdstream.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_XEN @@ -216,10 +217,10 @@ xenUnifiedProbe (void) return 1; #endif #ifdef __sun - FILE *fh; + int fd; - if (fh = fopen("/dev/xen/domcaps", "r")) { - fclose(fh); + if ((fd = open("/dev/xen/domcaps", O_RDONLY))>= 0) { + VIR_FORCE_CLOSE(fd); return 1; } #endif Index: libvirt-acl/src/xen/xen_hypervisor.c =================================================================== --- libvirt-acl.orig/src/xen/xen_hypervisor.c +++ libvirt-acl/src/xen/xen_hypervisor.c @@ -2641,7 +2641,7 @@ xenHypervisorMakeCapabilities(virConnect capabilities = fopen ("/sys/hypervisor/properties/capabilities", "r"); if (capabilities == NULL) { if (errno != ENOENT) { - fclose(cpuinfo); + VIR_FORCE_FCLOSE(cpuinfo); virReportSystemError(errno, _("cannot read file %s"), "/sys/hypervisor/properties/capabilities"); @@ -2654,10 +2654,8 @@ xenHypervisorMakeCapabilities(virConnect cpuinfo, capabilities); - if (cpuinfo) - fclose(cpuinfo); - if (capabilities) - fclose(capabilities); + VIR_FORCE_FCLOSE(cpuinfo); + VIR_FORCE_FCLOSE(capabilities); return caps; #endif /* __sun */ Index: libvirt-acl/tests/nodeinfotest.c =================================================================== --- libvirt-acl.orig/tests/nodeinfotest.c +++ libvirt-acl/tests/nodeinfotest.c @@ -9,6 +9,7 @@ #include "internal.h" #include "nodeinfo.h" #include "util.h" +#include "files.h" #ifndef __linux__ @@ -49,10 +50,10 @@ static int linuxTestCompareFiles(const c fprintf(stderr, "\n%s\n", error->message); virFreeError(error); } - fclose(cpuinfo); + VIR_FORCE_FCLOSE(cpuinfo); return -1; } - fclose(cpuinfo); + VIR_FORCE_FCLOSE(cpuinfo); /* 'nodes' is filled using libnuma.so from current machine * topology, which makes it unsuitable for the test suite Index: libvirt-acl/tests/testutils.c =================================================================== --- libvirt-acl.orig/tests/testutils.c +++ libvirt-acl/tests/testutils.c @@ -180,26 +180,26 @@ int virtTestLoadFile(const char *file, if (fstat(fileno(fp),&st)< 0) { fprintf (stderr, "%s: failed to fstat: %s\n", file, strerror(errno)); - fclose(fp); + VIR_FORCE_FCLOSE(fp); return -1; } if (st.st_size> (buflen-1)) { fprintf (stderr, "%s: larger than buffer (> %d)\n", file, buflen-1); - fclose(fp); + VIR_FORCE_FCLOSE(fp); return -1; } if (st.st_size) { if (fread(*buf, st.st_size, 1, fp) != 1) { fprintf (stderr, "%s: read failed: %s\n", file, strerror(errno)); - fclose(fp); + VIR_FORCE_FCLOSE(fp); return -1; } } (*buf)[st.st_size] = '\0'; - fclose(fp); + VIR_FORCE_FCLOSE(fp); return st.st_size; } Index: libvirt-acl/tests/xencapstest.c =================================================================== --- libvirt-acl.orig/tests/xencapstest.c +++ libvirt-acl/tests/xencapstest.c @@ -9,6 +9,7 @@ #include "xml.h" #include "testutils.h" #include "xen/xen_hypervisor.h" +#include "files.h" static char *progname; static char *abs_srcdir; @@ -63,10 +64,8 @@ static int testCompareFiles(const char * fail: free(actualxml); - if (fp1) - fclose(fp1); - if (fp2) - fclose(fp2); + VIR_FORCE_FCLOSE(fp1); + VIR_FORCE_FCLOSE(fp2); virCapabilitiesFree(caps); return ret; Index: libvirt-acl/docs/hacking.html.in =================================================================== --- libvirt-acl.orig/docs/hacking.html.in +++ libvirt-acl/docs/hacking.html.in @@ -414,25 +414,45 @@ <h2><a name="file_handling">File handling</a></h2> <p> - Use of the close() API is deprecated in libvirt code base to help - avoiding double-closing of a file descriptor. Instead of this API, - use the macro from files.h + Usage of the<code>fdopen()</code>,<code>close()</code>,<code>fclose()</code> + APIs is deprecated in libvirt code base to help avoiding double-closing of files + or file descriptors, which is particulary dangerous in a multi-threaded + applications. Instead of these APIs, use the macros from files.h </p> -<ul> +<ul> +<li><p>eg opening a file from a file descriptor</p> + +<pre> + if ((file = VIR_FDOPEN(fd, "r")) == NULL) { + virReportSystemError(errno, "%s", + _("failed to open file from file descriptor")); + return -1; + } + /* fd is now invalid; only access the file using file variable */ +</pre></li> + <li><p>e.g. close a file descriptor</p> <pre> if (VIR_CLOSE(fd)&lt; 0) { - virReportSystemError(errno, _("failed to close file")); + virReportSystemError(errno, "%s", _("failed to close file")); } -</pre> -</li> +</pre></li> + +<li><p>eg close a file</p> + +<pre> + if (VIR_FCLOSE(file)&lt; 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + } +</pre></li> -<li><p>eg close a file descriptor in an error path, without losing +<li><p>eg close a file or file descriptor in an error path, without losing the previous<code>errno</code> value</p> <pre> VIR_FORCE_CLOSE(fd); + VIR_FORCE_FCLOSE(file); </pre> </li> </ul> Index: libvirt-acl/HACKING =================================================================== --- libvirt-acl.orig/HACKING +++ libvirt-acl/HACKING @@ -339,22 +339,43 @@ routines, use the macros from memory.h File handling ============= -Use of the close() API is deprecated in libvirt code base to help avoiding -double-closing of a file descriptor. Instead of this API, use the macro from -files.h +Usage of the "fdopen()", "close()", "fclose()" APIs is deprecated in libvirt +code base to help avoiding double-closing of files or file descriptors, which +is particulary dangerous in a multi-threaded applications. Instead of these +APIs, use the macros from files.h + +- eg opening a file from a file descriptor + + if ((file = VIR_FDOPEN(fd, "r")) == NULL) { + virReportSystemError(errno, "%s", + _("failed to open file from file descriptor")); + return -1; + } + /* fd is now invalid; only access the file using file variable */ + + - e.g. close a file descriptor if (VIR_CLOSE(fd)< 0) { - virReportSystemError(errno, _("failed to close file")); + virReportSystemError(errno, "%s", _("failed to close file")); + } + + + +- eg close a file + + if (VIR_FCLOSE(file)< 0) { + virReportSystemError(errno, "%s", _("failed to close file")); } -- eg close a file descriptor in an error path, without losing the previous -"errno" value +- eg close a file or file descriptor in an error path, without losing the +previous "errno" value VIR_FORCE_CLOSE(fd); + VIR_FORCE_FCLOSE(file); Index: libvirt-acl/src/util/stats_linux.c =================================================================== --- libvirt-acl.orig/src/util/stats_linux.c +++ libvirt-acl/src/util/stats_linux.c @@ -25,6 +25,7 @@ # include "util.h" # include "stats_linux.h" # include "memory.h" +# include "files.h" # define VIR_FROM_THIS VIR_FROM_STATS_LINUX @@ -98,12 +99,12 @@ linuxDomainInterfaceStats(const char *pa stats->tx_packets = tx_packets; stats->tx_errs = tx_errs; stats->tx_drop = tx_drop; - fclose (fp); + VIR_FORCE_FCLOSE (fp); return 0; } } - fclose (fp); + VIR_FORCE_FCLOSE(fp); virStatsError(VIR_ERR_INTERNAL_ERROR, "/proc/net/dev: Interface not found"); Index: libvirt-acl/src/xen/block_stats.c =================================================================== --- libvirt-acl.orig/src/xen/block_stats.c +++ libvirt-acl/src/xen/block_stats.c @@ -27,6 +27,7 @@ # include "util.h" # include "block_stats.h" # include "memory.h" +# include "files.h" # define VIR_FROM_THIS VIR_FROM_STATS_LINUX @@ -100,7 +101,7 @@ read_stat (const char *path) /* read, but don't bail out before closing */ i = fread (str, 1, sizeof str - 1, fp); - if (fclose (fp) != 0 /* disk error */ + if (VIR_FCLOSE(fp) != 0 /* disk error */ || i< 1) /* ensure we read at least one byte */ return -1; ------------------------------------------------------------- --- /root/tmp/bak/libvirt-acl/src/openvz/openvz_conf.c 2010-11-15 17:03:15.962422965 -0500 +++ src/openvz/openvz_conf.c 2010-11-16 06:31:49.716482090 -0500 @@ -887,8 +887,9 @@ openvzSetDefinedUUID(int vpsid, unsigned char *uuid) { char *conf_file; char uuidstr[VIR_UUID_STRING_BUFLEN]; + FILE *fp = NULL; int ret = -1; if (uuid == NULL) return -1; @@ -899,9 +900,9 @@ if (openvzGetVPSUUID(vpsid, uuidstr, sizeof(uuidstr))) goto cleanup; if (uuidstr[0] == 0) { - FILE *fp = fopen(conf_file, "a"); /* append */ + fp = fopen(conf_file, "a"); /* append */ if (fp == NULL) goto cleanup; virUUIDFormat(uuid, uuidstr); @@ -914,8 +915,9 @@ } ret = 0; cleanup: + VIR_FORCE_FCLOSE(fp); VIR_FREE(conf_file); return ret; } --- /root/tmp/bak/libvirt-acl/src/util/files.c 2010-11-15 17:03:15.949423495 -0500 +++ src/util/files.c 2010-11-16 06:31:49.714482170 -0500 @@ -71,8 +71,10 @@ if (*fdptr>= 0) { file = fdopen(*fdptr, mode); if (file) *fdptr = -1; + } else { + errno = EBADF; } return file; } --- /root/tmp/bak/libvirt-acl/src/util/files.h 2010-11-15 17:03:15.956423210 -0500 +++ src/util/files.h 2010-11-16 06:31:49.714482170 -0500 @@ -32,17 +32,19 @@ # include "internal.h" # include "ignore-value.h" -/* Don't call this directly - use the macros below */ +/* Don't call these directly - use the macros below */ int virClose(int *fdptr, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; int virFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; -FILE *virFdopen(int *fdptr, const char *mode); +FILE *virFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; /* For use on normal paths; caller must check return value, and failure sets errno per close(). */ # define VIR_CLOSE(FD) virClose(&(FD), false) # define VIR_FCLOSE(FILE) virFclose(&(FILE), false) + +/* Wrapper around fdopen that consumes fd on success. */ # define VIR_FDOPEN(FD, MODE) virFdopen(&(FD), MODE) /* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ --- /root/tmp/bak/libvirt-acl/src/util/stats_linux.c 2010-11-15 17:03:15.994421659 -0500 +++ src/util/stats_linux.c 2010-11-16 06:31:49.724481766 -0500 @@ -98,9 +98,9 @@ stats->tx_bytes = tx_bytes; stats->tx_packets = tx_packets; stats->tx_errs = tx_errs; stats->tx_drop = tx_drop; - fclose (fp); + VIR_FORCE_FCLOSE (fp); return 0; } } --- /root/tmp/bak/libvirt-acl/src/xen/xen_driver.c 2010-11-15 17:03:15.972422557 -0500 +++ src/xen/xen_driver.c 2010-11-16 06:31:49.722481846 -0500 @@ -218,9 +218,9 @@ #endif #ifdef __sun int fd; - if (fd = open("/dev/xen/domcaps", O_RDONLY)) { + if ((fd = open("/dev/xen/domcaps", O_RDONLY))>= 0) { VIR_FORCE_CLOSE(fd); return 1; } #endif