Similarly to deprecating close(), I am now deprecating fclose() and introduce VIR_FORCE_FCLOSE() and VIR_FCLOSE(). 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 occurences of possible double-closed files on the way. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- HACKING | 22 ++++++++++++++++------ daemon/libvirtd.c | 4 ++-- docs/hacking.html.in | 20 +++++++++++++++----- src/libvirt_private.syms | 1 + src/nodeinfo.c | 8 ++++---- src/openvz/openvz_conf.c | 8 ++++---- src/openvz/openvz_driver.c | 4 ++-- src/qemu/qemu_driver.c | 4 ++-- src/storage/storage_backend.c | 6 ++---- src/storage/storage_backend_fs.c | 4 ++-- src/storage/storage_backend_iscsi.c | 7 ++----- 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 | 18 ++++++++++++++++++ src/util/files.h | 4 ++++ src/util/macvtap.c | 4 ++-- src/util/util.c | 8 +++----- src/xen/xen_driver.c | 3 ++- src/xen/xen_hypervisor.c | 8 +++----- tests/nodeinfotest.c | 5 +++-- tests/testutils.c | 8 ++++---- tests/xencapstest.c | 7 +++---- 24 files changed, 107 insertions(+), 71 deletions(-) Index: libvirt-acl/src/util/files.c =================================================================== --- libvirt-acl.orig/src/util/files.c +++ libvirt-acl/src/util/files.c @@ -44,3 +44,21 @@ 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; +} Index: libvirt-acl/src/util/files.h =================================================================== --- libvirt-acl.orig/src/util/files.h +++ libvirt-acl/src/util/files.h @@ -27,6 +27,7 @@ # define __VIR_FILES_H_ # include <stdbool.h> +# include <stdio.h> # include "internal.h" # include "ignore-value.h" @@ -34,13 +35,16 @@ /* Don't call this 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; /* 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) /* 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,7 @@ virFDStreamCreateFile; # files.h virClose; +virFClose; # hash.h Index: libvirt-acl/daemon/libvirtd.c =================================================================== --- libvirt-acl.orig/daemon/libvirtd.c +++ libvirt-acl/daemon/libvirtd.c @@ -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; @@ -909,7 +909,7 @@ openvzSetDefinedUUID(int vpsid, unsigned /* Record failure if fprintf or fclose fails, and be careful always to close the stream. */ if ((fprintf(fp, "\n#UUID: %s\n", uuidstr) < 0) - + (fclose(fp) == EOF)) + + (VIR_FCLOSE(fp) == EOF)) goto cleanup; } @@ -996,7 +996,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 @@ -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); 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 @@ -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 @@ -436,11 +436,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 @@ -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 @@ -219,7 +220,7 @@ xenUnifiedProbe (void) FILE *fh; if (fh = fopen("/dev/xen/domcaps", "r")) { - fclose(fh); + VIR_FORCE_FCLOSE(fh); 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/HACKING =================================================================== --- libvirt-acl.orig/HACKING +++ libvirt-acl/HACKING @@ -321,20 +321,30 @@ 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 close() and fclose() APIs is deprecated in libvirt code base to help +avoiding double-closing of files or file descriptors, which is particulary +dangerous in multi-threaded applications. Instead of these APIs, use the +macros from files.h - eg 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 descriptor in an error path, without losing the previous - errno value + - eg close a file + + if (VIR_FCLOSE(file) < 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + } + + - 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); + + String comparisons ================== Index: libvirt-acl/docs/hacking.html.in =================================================================== --- libvirt-acl.orig/docs/hacking.html.in +++ libvirt-acl/docs/hacking.html.in @@ -392,9 +392,10 @@ <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 close() and 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 </p> <ul> @@ -402,15 +403,24 @@ <pre> if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, _("failed to close file")); + 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</p> + +<pre> + if (VIR_FCLOSE(file) < 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + } +</pre></li> + + <li><p>eg close a file or file descriptor in an error path, without losing the previous errno value</p> <pre> VIR_FORCE_CLOSE(fd); + VIR_FORCE_FCLOSE(file); </pre></li> </ul>