[libvirt] [PATCH] bye to close(), welcome to VIR_(FORCE_)CLOSE()
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@us.ibm.com> --- daemon/libvirtd.c | 46 ++++++--------- proxy/libvirt_proxy.c | 16 ++--- src/libvirt.c | 8 +- src/lxc/lxc_container.c | 15 +++-- src/lxc/lxc_controller.c | 27 +++------ src/lxc/lxc_driver.c | 24 +++----- src/node_device/node_device_linux_sysfs.c | 5 - src/nwfilter/nwfilter_ebiptables_driver.c | 8 +- src/openvz/openvz_conf.c | 35 ++++------- src/openvz/openvz_driver.c | 3 - src/phyp/phyp_driver.c | 13 ++-- src/qemu/qemu_conf.c | 30 ++++------ src/qemu/qemu_driver.c | 89 ++++++++++++------------------ src/qemu/qemu_monitor.c | 6 +- src/remote/remote_driver.c | 18 ++---- src/secret/secret_driver.c | 12 +--- src/security/security_apparmor.c | 9 +-- src/security/security_selinux.c | 9 +-- src/security/virt-aa-helper.c | 9 +-- src/storage/storage_backend.c | 25 +++----- src/storage/storage_backend_fs.c | 11 ++- src/storage/storage_backend_iscsi.c | 5 - src/storage/storage_backend_logical.c | 10 +-- src/storage/storage_backend_mpath.c | 5 - src/storage/storage_backend_scsi.c | 8 +- src/storage/storage_driver.c | 5 - src/test/test_driver.c | 20 ++---- src/uml/uml_conf.c | 3 - src/uml/uml_driver.c | 24 ++++---- src/util/bridge.c | 13 ++-- src/util/conf.c | 3 - src/util/hooks.c | 21 +++---- src/util/interface.c | 12 ++-- src/util/logging.c | 6 +- src/util/macvtap.c | 8 +- src/util/pci.c | 6 -- src/util/storage_file.c | 5 + src/util/util.c | 67 ++++++++++------------ src/util/uuid.c | 9 +-- src/util/virtaudit.c | 3 - src/xen/proxy_internal.c | 8 +- src/xen/xen_hypervisor.c | 12 ++-- src/xen/xen_inotify.c | 3 - src/xen/xend_internal.c | 12 +--- tests/testutils.c | 21 ++++--- tools/console.c | 3 - 46 files changed, 327 insertions(+), 383 deletions(-) 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); * * 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); * * * Returns the number of bytes read, which may be less @@ -10964,7 +10964,7 @@ error: * if (virStreamFinish(st) < 0) * ...report an error... * virStreamFree(st); - * close(fd); + * VIR_FORCE_CLOSE(fd); * * Returns 0 if all the data was successfully sent. The caller * should invoke virStreamFinish(st) to flush the stream upon @@ -11061,7 +11061,7 @@ cleanup: * if (virStreamFinish(st) < 0) * ...report an error... * virStreamFree(st); - * close(fd); + * VIR_FORCE_CLOSE(fd); * * Returns 0 if all the data was successfully received. The caller * should invoke virStreamFinish(st) to flush the stream upon Index: libvirt-acl/src/lxc/lxc_container.c =================================================================== --- libvirt-acl.orig/src/lxc/lxc_container.c +++ libvirt-acl/src/lxc/lxc_container.c @@ -52,6 +52,7 @@ #include "util.h" #include "memory.h" #include "veth.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -127,7 +128,7 @@ static int lxcContainerExecInit(virDomai static int lxcContainerSetStdio(int control, int ttyfd) { int rc = -1; - int open_max, i; + int open_max, i, tpmfd; if (setsid() < 0) { virReportSystemError(errno, "%s", @@ -145,8 +146,10 @@ static int lxcContainerSetStdio(int cont * close all FDs before executing the container */ open_max = sysconf (_SC_OPEN_MAX); for (i = 0; i < open_max; i++) - if (i != ttyfd && i != control) - close(i); + if (i != ttyfd && i != control) { + tpmfd = i; + VIR_FORCE_CLOSE(tpmfd); + } if (dup2(ttyfd, 0) < 0) { virReportSystemError(errno, "%s", @@ -222,7 +225,7 @@ static int lxcContainerWaitForContinue(i _("Failed to read the container continue message")); return -1; } - close(control); + VIR_FORCE_CLOSE(control); DEBUG0("Received container continue message"); @@ -776,10 +779,10 @@ static int lxcContainerChild( void *data VIR_FREE(ttyPath); if (lxcContainerSetStdio(argv->monitor, ttyfd) < 0) { - close(ttyfd); + VIR_FORCE_CLOSE(ttyfd); return -1; } - close(ttyfd); + VIR_FORCE_CLOSE(ttyfd); if (lxcContainerSetupMounts(vmDef, root) < 0) return -1; Index: libvirt-acl/src/lxc/lxc_controller.c =================================================================== --- libvirt-acl.orig/src/lxc/lxc_controller.c +++ libvirt-acl/src/lxc/lxc_controller.c @@ -48,6 +48,7 @@ #include "veth.h" #include "memory.h" #include "util.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -233,8 +234,7 @@ static int lxcMonitorServer(const char * return fd; error: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -409,7 +409,7 @@ static int lxcControllerMain(int monitor goto cleanup; } if (client != -1) { /* Already connected, so kick new one out */ - close(fd); + VIR_FORCE_CLOSE(fd); continue; } client = fd; @@ -426,8 +426,7 @@ static int lxcControllerMain(int monitor _("epoll_ctl(client) failed")); goto cleanup; } - close(client); - client = -1; + VIR_FORCE_CLOSE(client); } else { if (epollEvent.events & EPOLLIN) { curFdOff = epollEvent.data.fd == appPty ? 0 : 1; @@ -485,9 +484,9 @@ static int lxcControllerMain(int monitor rc = 0; cleanup: - close(appPty); - close(contPty); - close(epollFd); + VIR_FORCE_CLOSE(appPty); + VIR_FORCE_CLOSE(contPty); + VIR_FORCE_CLOSE(epollFd); return rc; } @@ -660,8 +659,7 @@ lxcControllerRun(virDomainDefPtr def, control[1], containerPtyPath)) < 0) goto cleanup; - close(control[1]); - control[1] = -1; + VIR_FORCE_CLOSE(control[1]); if (lxcControllerMoveInterfaces(nveths, veths, container) < 0) goto cleanup; @@ -679,13 +677,10 @@ lxcControllerRun(virDomainDefPtr def, cleanup: VIR_FREE(devptmx); VIR_FREE(devpts); - if (control[0] != -1) - close(control[0]); - if (control[1] != -1) - close(control[1]); + VIR_FORCE_CLOSE(control[0]); + VIR_FORCE_CLOSE(control[1]); VIR_FREE(containerPtyPath); - if (containerPty != -1) - close(containerPty); + VIR_FORCE_CLOSE(containerPty); if (container > 1) { int status; Index: libvirt-acl/src/lxc/lxc_driver.c =================================================================== --- libvirt-acl.orig/src/lxc/lxc_driver.c +++ libvirt-acl/src/lxc/lxc_driver.c @@ -51,6 +51,7 @@ #include "uuid.h" #include "stats_linux.h" #include "hooks.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -974,7 +975,7 @@ static int lxcVmCleanup(lxc_driver_t *dr } virEventRemoveHandle(priv->monitorWatch); - close(priv->monitor); + VIR_FORCE_CLOSE(priv->monitor); virFileDeletePid(driver->stateDir, vm->def->name); virDomainDeleteConfig(driver->stateDir, NULL, vm); @@ -1156,8 +1157,7 @@ static int lxcMonitorClient(lxc_driver_t error: VIR_FREE(sockpath); - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -1544,14 +1544,10 @@ cleanup: vethDelete(veths[i]); VIR_FREE(veths[i]); } - if (rc != 0 && priv->monitor != -1) { - close(priv->monitor); - priv->monitor = -1; - } - if (parentTty != -1) - close(parentTty); - if (logfd != -1) - close(logfd); + if (rc != 0) + VIR_FORCE_CLOSE(priv->monitor); + VIR_FORCE_CLOSE(parentTty); + VIR_FORCE_CLOSE(logfd); VIR_FREE(logfile); return rc; } @@ -2011,8 +2007,7 @@ lxcReconnectVM(void *payload, const char /* Read pid from controller */ if ((virFileReadPid(lxc_driver->stateDir, vm->def->name, &vm->pid)) != 0) { - close(priv->monitor); - priv->monitor = -1; + VIR_FORCE_CLOSE(priv->monitor); goto cleanup; } @@ -2042,8 +2037,7 @@ lxcReconnectVM(void *payload, const char } } else { vm->def->id = -1; - close(priv->monitor); - priv->monitor = -1; + VIR_FORCE_CLOSE(priv->monitor); } cleanup: Index: libvirt-acl/src/node_device/node_device_linux_sysfs.c =================================================================== --- libvirt-acl.orig/src/node_device/node_device_linux_sysfs.c +++ libvirt-acl/src/node_device/node_device_linux_sysfs.c @@ -31,6 +31,7 @@ #include "virterror_internal.h" #include "memory.h" #include "logging.h" +#include "files.h" #include <dirent.h> #define VIR_FROM_THIS VIR_FROM_NODEDEV @@ -104,9 +105,7 @@ int read_wwn_linux(int host, const char } out: - if (fd != -1) { - close(fd); - } + VIR_FORCE_CLOSE(fd); return retval; } Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -37,6 +37,7 @@ #include "nwfilter_conf.h" #include "nwfilter_gentech_driver.h" #include "nwfilter_ebiptables_driver.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -2501,13 +2502,12 @@ ebiptablesWriteToTempFile(const char *st } VIR_FREE(header); - close(fd); + VIR_FORCE_CLOSE(fd); return filnam; err_exit: VIR_FREE(header); - if (fd >= 0) - close(fd); + VIR_FORCE_CLOSE(fd); unlink(filename); return NULL; } @@ -3267,7 +3267,7 @@ iptablesCheckBridgeNFCallEnabled(bool is lastReport = now; } } - close(fd); + VIR_FORCE_CLOSE(fd); } } } Index: libvirt-acl/src/openvz/openvz_conf.c =================================================================== --- libvirt-acl.orig/src/openvz/openvz_conf.c +++ libvirt-acl/src/openvz/openvz_conf.c @@ -50,6 +50,7 @@ #include "memory.h" #include "util.h" #include "nodeinfo.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -109,7 +110,7 @@ openvzExtractVersionInfo(const char *cmd cleanup2: VIR_FREE(help); - if (close(newstdout) < 0) + if (VIR_CLOSE(newstdout) < 0) ret = -1; rewait: @@ -569,7 +570,7 @@ openvzWriteConfigParam(const char * conf goto error; temp_fd = open(temp_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); if (temp_fd == -1) { - close(fd); + VIR_FORCE_CLOSE(fd); goto error; } @@ -590,12 +591,10 @@ openvzWriteConfigParam(const char * conf safewrite(temp_fd, "\"\n", 2) < 0) goto error; - if (close(fd) < 0) + if (VIR_CLOSE(fd) < 0) goto error; - fd = -1; - if (close(temp_fd) < 0) + if (VIR_CLOSE(temp_fd) < 0) goto error; - temp_fd = -1; if (rename(temp_file, conf_file) < 0) goto error; @@ -603,10 +602,8 @@ openvzWriteConfigParam(const char * conf return 0; error: - if (fd != -1) - close(fd); - if (temp_fd != -1) - close(temp_fd); + VIR_FORCE_CLOSE(fd); + VIR_FORCE_CLOSE(temp_fd); if (temp_file) unlink(temp_file); VIR_FREE(temp_file); @@ -662,7 +659,7 @@ openvzReadConfigParam(const char * conf_ } } } - close(fd); + VIR_FORCE_CLOSE(fd); if (ret == 0 && found) ret = 1; @@ -703,7 +700,7 @@ openvz_copyfile(char* from_path, char* t return -1; copy_fd = open(to_path, O_WRONLY | O_CREAT | O_TRUNC, 0644); if (copy_fd == -1) { - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -716,19 +713,16 @@ openvz_copyfile(char* from_path, char* t goto error; } - if (close(fd) < 0) + if (VIR_CLOSE(fd) < 0) goto error; - fd = -1; - if (close(copy_fd) < 0) + if (VIR_CLOSE(copy_fd) < 0) goto error; return 0; error: - if (fd != -1) - close(fd); - if (copy_fd != -1) - close(copy_fd); + VIR_FORCE_CLOSE(fd); + VIR_FORCE_CLOSE(copy_fd); return -1; } @@ -880,8 +874,7 @@ openvzGetVPSUUID(int vpsid, char *uuidst } retval = 0; cleanup: - if (fd >= 0) - close(fd); + VIR_FORCE_CLOSE(fd); VIR_FREE(conf_file); return retval; Index: libvirt-acl/src/openvz/openvz_driver.c =================================================================== --- libvirt-acl.orig/src/openvz/openvz_driver.c +++ libvirt-acl/src/openvz/openvz_driver.c @@ -57,6 +57,7 @@ #include "nodeinfo.h" #include "memory.h" #include "bridge.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -1540,7 +1541,7 @@ Version: 2.2 } } - close(fd); + VIR_FORCE_CLOSE(fd); if (ret < 0) return -1; Index: libvirt-acl/src/phyp/phyp_driver.c =================================================================== --- libvirt-acl.orig/src/phyp/phyp_driver.c +++ libvirt-acl/src/phyp/phyp_driver.c @@ -58,6 +58,7 @@ #include "domain_conf.h" #include "storage_conf.h" #include "nodeinfo.h" +#include "files.h" #include "phyp_driver.h" @@ -457,11 +458,11 @@ phypUUIDTable_WriteFile(virConnectPtr co } } - close(fd); + VIR_FORCE_CLOSE(fd); return 0; err: - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -672,11 +673,11 @@ phypUUIDTable_ReadFile(virConnectPtr con } else virReportOOMError(); - close(fd); + VIR_FORCE_CLOSE(fd); return 0; err: - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -764,7 +765,7 @@ phypUUIDTable_Pull(virConnectPtr conn) } break; } - close(fd); + VIR_FORCE_CLOSE(fd); goto exit; exit: @@ -1001,7 +1002,7 @@ openSSHSession(virConnectPtr conn, virCo if (connect(sock, cur->ai_addr, cur->ai_addrlen) == 0) { goto connected; } - close(sock); + VIR_FORCE_CLOSE(sock); } cur = cur->ai_next; } Index: libvirt-acl/src/qemu/qemu_conf.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_conf.c +++ libvirt-acl/src/qemu/qemu_conf.c @@ -55,6 +55,7 @@ #include "macvtap.h" #include "cpu/cpu.h" #include "domain_nwfilter.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -530,7 +531,7 @@ qemudProbeMachineTypes(const char *binar cleanup2: VIR_FREE(output); cleanup: - if (close(newstdout) < 0) + if (VIR_CLOSE(newstdout) < 0) ret = -1; rewait: @@ -780,7 +781,7 @@ qemudProbeCPUModels(const char *qemu, cleanup: VIR_FREE(output); - if (close(newstdout) < 0) + if (VIR_CLOSE(newstdout) < 0) ret = -1; rewait: @@ -1421,7 +1422,7 @@ static void qemudParsePCIDeviceStrs(cons cleanup: VIR_FREE(pciassign); - close(newstderr); + VIR_FORCE_CLOSE(newstderr); rewait: if (waitpid(child, &status, 0) != child) { if (errno == EINTR) @@ -1481,7 +1482,7 @@ int qemudExtractVersionInfo(const char * cleanup2: VIR_FREE(help); - if (close(newstdout) < 0) + if (VIR_CLOSE(newstdout) < 0) ret = -1; rewait: @@ -1596,8 +1597,7 @@ qemudPhysIfaceConnect(virConnectPtr conn if ((net->filter) && (net->ifname)) { err = virDomainConfNWFilterInstantiate(conn, net); if (err) { - close(rc); - rc = -1; + VIR_FORCE_CLOSE(rc); delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, &net->data.direct.virtPortProfile); VIR_FREE(net->ifname); @@ -1742,10 +1742,8 @@ qemudNetworkIfaceConnect(virConnectPtr c if (tapfd >= 0) { if ((net->filter) && (net->ifname)) { err = virDomainConfNWFilterInstantiate(conn, net); - if (err) { - close(tapfd); - tapfd = -1; - } + if (err) + VIR_FORCE_CLOSE(tapfd); } } @@ -4557,7 +4555,7 @@ int qemudBuildCommandLine(virConnectPtr if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { virDomainConfNWFilterTeardown(net); - close(tapfd); + VIR_FORCE_CLOSE(tapfd); goto no_memory; } @@ -4576,7 +4574,7 @@ int qemudBuildCommandLine(virConnectPtr if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { virDomainConfNWFilterTeardown(net); - close(tapfd); + VIR_FORCE_CLOSE(tapfd); goto no_memory; } @@ -4596,7 +4594,7 @@ int qemudBuildCommandLine(virConnectPtr int vhostfd = qemudOpenVhostNet(net, qemuCmdFlags); if (vhostfd >= 0) { if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { - close(vhostfd); + VIR_FORCE_CLOSE(vhostfd); goto no_memory; } @@ -5094,14 +5092,14 @@ int qemudBuildCommandLine(virConnectPtr if (configfd >= 0) { if (virAsprintf(&configfd_name, "%d", configfd) < 0) { - close(configfd); + VIR_FORCE_CLOSE(configfd); virReportOOMError(); goto no_memory; } if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { VIR_FREE(configfd_name); - close(configfd); + VIR_FORCE_CLOSE(configfd); goto no_memory; } @@ -5194,7 +5192,7 @@ int qemudBuildCommandLine(virConnectPtr if (vmfds && *vmfds) { for (i = 0; i < *nvmfds; i++) - close((*vmfds)[i]); + VIR_FORCE_CLOSE((*vmfds)[i]); VIR_FREE(*vmfds); *nvmfds = 0; } Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -81,6 +81,7 @@ #include "hooks.h" #include "storage_file.h" #include "virtaudit.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -761,7 +762,7 @@ qemudLogFD(struct qemud_driver *driver, if (virSetCloseExec(fd) < 0) { virReportSystemError(errno, "%s", _("Unable to set VM logfile close-on-exec flag")); - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } return fd; @@ -793,14 +794,14 @@ qemudLogReadFD(const char* logDir, const if (virSetCloseExec(fd) < 0) { virReportSystemError(errno, "%s", _("Unable to set VM logfile close-on-exec flag")); - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } if (pos < 0 || lseek(fd, pos, SEEK_SET) < 0) { - virReportSystemError(pos < 0 ? 0 : errno, + virReportSystemError(pos < 0 ? 0 : errno, _("Unable to seek to %lld in %s"), (long long) pos, logfile); - close(fd); + VIR_FORCE_CLOSE(fd); } return fd; } @@ -2392,7 +2393,7 @@ cleanup: } closelog: - if (close(logfd) < 0) { + if (VIR_CLOSE(logfd) < 0) { char ebuf[4096]; VIR_WARN("Unable to close logfile: %s", virStrerror(errno, ebuf, sizeof ebuf)); @@ -2971,13 +2972,13 @@ static int qemudNextFreeVNCPort(struct q return -1; if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (void*)&reuse, sizeof(reuse)) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); break; } if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0) { /* Not in use, lets grab it */ - close(fd); + VIR_FORCE_CLOSE(fd); /* Add port to bitmap of reserved ports */ if (virBitmapSetBit(driver->reservedVNCPorts, i - QEMU_VNC_PORT_MIN) < 0) { @@ -2986,7 +2987,7 @@ static int qemudNextFreeVNCPort(struct q } return i; } - close(fd); + VIR_FORCE_CLOSE(fd); if (errno == EADDRINUSE) { /* In use, try next */ @@ -3238,7 +3239,7 @@ qemuPrepareChardevDevice(virDomainDefPtr return -1; } - close(fd); + VIR_FORCE_CLOSE(fd); return 0; } @@ -3955,7 +3956,7 @@ static int qemudStartVMDaemon(virConnect if (vmfds) { for (i = 0 ; i < nvmfds ; i++) { - close(vmfds[i]); + VIR_FORCE_CLOSE(vmfds[i]); } VIR_FREE(vmfds); } @@ -4008,8 +4009,7 @@ static int qemudStartVMDaemon(virConnect if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) goto cleanup; - if (logfile != -1) - close(logfile); + VIR_FORCE_CLOSE(logfile); return 0; @@ -4019,8 +4019,7 @@ cleanup: * pretend we never started it */ qemudShutdownVMDaemon(driver, vm, 0); - if (logfile != -1) - close(logfile); + VIR_FORCE_CLOSE(logfile); return -1; } @@ -4295,7 +4294,7 @@ static int kvmGetMaxVCPUs(void) { if (r > 0) maxvcpus = r; - close(fd); + VIR_FORCE_CLOSE(fd); return maxvcpus; } @@ -5397,10 +5396,10 @@ static int qemudDomainSaveFlag(struct qe goto endjob; } if (qemudDomainSaveFileOpHook(fd, &hdata) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); goto endjob; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to close %s"), path); goto endjob; } @@ -5796,7 +5795,7 @@ static int qemudDomainCoreDump(virDomain goto endjob; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to save file %s"), path); @@ -6424,8 +6423,7 @@ static int qemudOpenAsUID(const char *pa /* parent */ /* parent doesn't need the write side of the pipe */ - close(pipefd[1]); - pipefd[1] = -1; + VIR_FORCE_CLOSE(pipefd[1]); if (forkRet < 0) { virReportSystemError(errno, @@ -6437,10 +6435,8 @@ static int qemudOpenAsUID(const char *pa fd = pipefd[0]; pipefd[0] = -1; parent_cleanup: - if (pipefd[0] != -1) - close(pipefd[0]); - if (pipefd[1] != -1) - close(pipefd[1]); + VIR_FORCE_CLOSE(pipefd[0]); + VIR_FORCE_CLOSE(pipefd[1]); if ((fd < 0) && (*child_pid > 0)) { /* a child process was started and subsequently an error occurred in the parent, so we need to wait for it to @@ -6466,7 +6462,7 @@ parent_cleanup: struct passwd pwd, *pwd_result; /* child doesn't need the read side of the pipe */ - close(pipefd[0]); + VIR_FORCE_CLOSE(pipefd[0]); if (forkRet < 0) { exit_code = errno; @@ -6531,10 +6527,8 @@ parent_cleanup: child_cleanup: VIR_FREE(buf); - if (fd != -1) - close(fd); - if (pipefd[1] != -1) - close(pipefd[1]); + VIR_FORCE_CLOSE(fd); + VIR_FORCE_CLOSE(pipefd[1]); _exit(exit_code); } @@ -6542,8 +6536,10 @@ static int qemudDomainSaveImageClose(int { int ret = 0; - if (fd != -1) - close(fd); + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, "%s", + _("cannot close file")); + } if (read_pid != -1) { /* reap the process that read the file */ @@ -6699,8 +6695,7 @@ qemudDomainSaveImageStartVM(virConnectPt /* empty */ } } - if (intermediatefd != -1) - close(intermediatefd); + VIR_FORCE_CLOSE(intermediatefd); wait_ret = qemudDomainSaveImageClose(fd, read_pid, &status); fd = -1; @@ -8065,9 +8060,7 @@ static int qemudDomainAttachNetDevice(vi } qemuDomainObjExitMonitorWithDriver(driver, vm); - if (tapfd != -1) - close(tapfd); - tapfd = -1; + VIR_FORCE_CLOSE(tapfd); if (!virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -8117,8 +8110,7 @@ cleanup: VIR_FREE(nicstr); VIR_FREE(netstr); VIR_FREE(tapfd_name); - if (tapfd != -1) - close(tapfd); + VIR_FORCE_CLOSE(tapfd); return ret; @@ -8247,8 +8239,7 @@ static int qemudDomainAttachHostPciDevic VIR_FREE(devstr); VIR_FREE(configfd_name); - if (configfd >= 0) - close(configfd); + VIR_FORCE_CLOSE(configfd); return 0; @@ -8262,8 +8253,7 @@ error: VIR_FREE(devstr); VIR_FREE(configfd_name); - if (configfd >= 0) - close(configfd); + VIR_FORCE_CLOSE(configfd); return -1; } @@ -10357,8 +10347,7 @@ static int qemuDomainGetBlockInfo(virDom } cleanup: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); if (vm) virDomainObjUnlock(vm); return ret; @@ -10673,8 +10662,7 @@ cleanup: static void qemuStreamMigFree(struct qemuStreamMigFile *qemust) { - if (qemust->fd != -1) - close(qemust->fd); + VIR_FORCE_CLOSE(qemust->fd); VIR_FREE(qemust); } @@ -11510,10 +11498,8 @@ finish: qemuDomainObjExitRemoteWithDriver(driver, vm); cleanup: - if (client_sock != -1) - close(client_sock); - if (qemu_sock != -1) - close(qemu_sock); + VIR_FORCE_CLOSE(client_sock); + VIR_FORCE_CLOSE(qemu_sock); if (ddomain) virUnrefDomain(ddomain); @@ -12292,8 +12278,7 @@ cleanup: VIR_FREE(snapFile); VIR_FREE(snapDir); VIR_FREE(newxml); - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } Index: libvirt-acl/src/qemu/qemu_monitor.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_monitor.c +++ libvirt-acl/src/qemu/qemu_monitor.c @@ -36,6 +36,7 @@ #include "virterror_internal.h" #include "memory.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -283,7 +284,7 @@ qemuMonitorOpenUnix(const char *monitor) return monfd; error: - close(monfd); + VIR_FORCE_CLOSE(monfd); return -1; } @@ -694,8 +695,7 @@ void qemuMonitorClose(qemuMonitorPtr mon if (!mon->closed) { if (mon->watch) virEventRemoveHandle(mon->watch); - if (mon->fd != -1) - close(mon->fd); + VIR_FORCE_CLOSE(mon->fd); /* NB: ordinarily one might immediately set mon->watch to -1 * and mon->fd to -1, but there may be a callback active * that is still relying on these fields being valid. So Index: libvirt-acl/src/remote/remote_driver.c =================================================================== --- libvirt-acl.orig/src/remote/remote_driver.c +++ libvirt-acl/src/remote/remote_driver.c @@ -82,6 +82,7 @@ #include "util.h" #include "event.h" #include "ignore-value.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_REMOTE @@ -711,7 +712,7 @@ doRemoteOpen (virConnectPtr conn, if (errno == ECONNREFUSED && flags & VIR_DRV_OPEN_REMOTE_AUTOSTART && trials < 20) { - close(priv->sock); + VIR_FORCE_CLOSE(priv->sock); priv->sock = -1; if (trials > 0 || remoteForkDaemon() == 0) { @@ -955,8 +956,7 @@ doRemoteOpen (virConnectPtr conn, failed: /* Close the socket if we failed. */ - if (priv->errfd >= 0) - close(priv->errfd); + VIR_FORCE_CLOSE(priv->errfd); if (priv->sock >= 0) { if (priv->uses_tls && priv->session) { @@ -977,10 +977,8 @@ retry: #endif } - if (wakeupFD[0] >= 0) { - close(wakeupFD[0]); - close(wakeupFD[1]); - } + VIR_FORCE_CLOSE(wakeupFD[0]); + VIR_FORCE_CLOSE(wakeupFD[1]); VIR_FREE(priv->hostname); goto cleanup; @@ -1456,10 +1454,8 @@ retry: } while (reap != -1 && reap != priv->pid); } #endif - if (priv->wakeupReadFD >= 0) { - close(priv->wakeupReadFD); - close(priv->wakeupSendFD); - } + VIR_FORCE_CLOSE(priv->wakeupReadFD); + VIR_FORCE_CLOSE(priv->wakeupSendFD); /* Free hostname copy */ Index: libvirt-acl/src/secret/secret_driver.c =================================================================== --- libvirt-acl.orig/src/secret/secret_driver.c +++ libvirt-acl/src/secret/secret_driver.c @@ -41,6 +41,7 @@ #include "util.h" #include "uuid.h" #include "virterror_internal.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -181,7 +182,7 @@ replaceFile(const char *filename, void * tmp_path); goto cleanup; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("error closing '%s'"), tmp_path); goto cleanup; } @@ -196,8 +197,7 @@ replaceFile(const char *filename, void * ret = 0; cleanup: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); if (tmp_path != NULL) { unlink(tmp_path); VIR_FREE(tmp_path); @@ -394,8 +394,7 @@ secretLoadValue(virSecretDriverStatePtr virReportSystemError(errno, _("cannot read '%s'"), filename); goto cleanup; } - close(fd); - fd = -1; + VIR_FORCE_CLOSE(fd); if (!base64_decode_alloc(contents, st.st_size, &value, &value_size)) { virSecretReportError(VIR_ERR_INTERNAL_ERROR, @@ -422,8 +421,7 @@ cleanup: memset(contents, 0, st.st_size); VIR_FREE(contents); } - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); VIR_FREE(filename); return ret; } Index: libvirt-acl/src/security/security_apparmor.c =================================================================== --- libvirt-acl.orig/src/security/security_apparmor.c +++ libvirt-acl/src/security/security_apparmor.c @@ -37,6 +37,7 @@ #include "uuid.h" #include "pci.h" #include "hostusb.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_APPARMOR_VOID_DOI "0" @@ -215,7 +216,7 @@ load_profile(virSecurityDriverPtr drv, virReportSystemError(errno, "%s", _("unable to write to pipe")); goto clean; } - close(pipefd[1]); + VIR_FORCE_CLOSE(pipefd[1]); rc = 0; rewait: @@ -233,10 +234,8 @@ load_profile(virSecurityDriverPtr drv, clean: VIR_FREE(xml); - if (pipefd[0] > 0) - close(pipefd[0]); - if (pipefd[1] > 0) - close(pipefd[1]); + VIR_FORCE_CLOSE(pipefd[0]); + VIR_FORCE_CLOSE(pipefd[1]); return rc; } Index: libvirt-acl/src/security/security_selinux.c =================================================================== --- libvirt-acl.orig/src/security/security_selinux.c +++ libvirt-acl/src/security/security_selinux.c @@ -30,6 +30,7 @@ #include "storage_file.h" #include "uuid.h" #include "virtaudit.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_SECURITY @@ -122,10 +123,10 @@ SELinuxInitialize(void) virReportSystemError(errno, _("cannot read SELinux virtual domain context file %s"), selinux_virtual_domain_context_path()); - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } - close(fd); + VIR_FORCE_CLOSE(fd); ptr = strchrnul(default_domain_context, '\n'); *ptr = '\0'; @@ -141,10 +142,10 @@ SELinuxInitialize(void) virReportSystemError(errno, _("cannot read SELinux virtual image context file %s"), selinux_virtual_image_context_path()); - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } - close(fd); + VIR_FORCE_CLOSE(fd); ptr = strchrnul(default_image_context, '\n'); if (*ptr == '\n') { Index: libvirt-acl/src/security/virt-aa-helper.c =================================================================== --- libvirt-acl.orig/src/security/virt-aa-helper.c +++ libvirt-acl/src/security/virt-aa-helper.c @@ -37,6 +37,7 @@ #include "uuid.h" #include "hostusb.h" #include "pci.h" +#include "files.h" static char *progname; @@ -278,12 +279,12 @@ update_include_file(const char *include_ } if (safewrite(fd, pcontent, plen) < 0) { /* don't write the '\0' */ - close(fd); + VIR_FORCE_CLOSE(fd); vah_error(NULL, 0, "failed to write to profile"); goto clean; } - if (close(fd) != 0) { + if (VIR_CLOSE(fd) != 0) { vah_error(NULL, 0, "failed to close or write to profile"); goto clean; } @@ -385,12 +386,12 @@ create_profile(const char *profile, cons } if (safewrite(fd, pcontent, plen - 1) < 0) { /* don't write the '\0' */ - close(fd); + VIR_FORCE_CLOSE(fd); vah_error(NULL, 0, "failed to write to profile"); goto clean_all; } - if (close(fd) != 0) { + if (VIR_CLOSE(fd) != 0) { vah_error(NULL, 0, "failed to close or write to profile"); goto clean_all; } Index: libvirt-acl/src/storage/storage_backend.c =================================================================== --- libvirt-acl.orig/src/storage/storage_backend.c +++ libvirt-acl/src/storage/storage_backend.c @@ -51,6 +51,7 @@ #include "storage_file.h" #include "storage_backend.h" #include "logging.h" +#include "files.h" #if WITH_STORAGE_LVM # include "storage_backend_logical.h" @@ -181,7 +182,7 @@ virStorageBackendCopyToFD(virStorageVolD } while ((amtleft -= 512) > 0); } - if (inputfd != -1 && close(inputfd) < 0) { + if (VIR_CLOSE(inputfd) < 0) { ret = -errno; virReportSystemError(errno, _("cannot close file '%s'"), @@ -193,8 +194,7 @@ virStorageBackendCopyToFD(virStorageVolD *total -= remain; cleanup: - if (inputfd != -1) - close(inputfd); + VIR_FORCE_CLOSE(inputfd); VIR_FREE(buf); @@ -251,7 +251,7 @@ virStorageBackendCreateBlockFrom(virConn vol->target.path, vol->target.perms.mode); goto cleanup; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot close file '%s'"), vol->target.path); @@ -261,8 +261,7 @@ virStorageBackendCreateBlockFrom(virConn ret = 0; cleanup: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } @@ -608,7 +607,7 @@ static int virStorageBackendQEMUImgBacki cleanup: VIR_FREE(help); - close(newstdout); + VIR_FORCE_CLOSE(newstdout); rewait: if (child) { if (waitpid(child, &status, 0) != child) { @@ -997,7 +996,7 @@ virStorageBackendVolOpenCheckMode(const virReportSystemError(errno, _("cannot stat file '%s'"), path); - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -1009,7 +1008,7 @@ virStorageBackendVolOpenCheckMode(const mode = VIR_STORAGE_VOL_OPEN_BLOCK; if (!(mode & flags)) { - close(fd); + VIR_FORCE_CLOSE(fd); if (mode & VIR_STORAGE_VOL_OPEN_ERROR) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, @@ -1045,7 +1044,7 @@ virStorageBackendUpdateVolTargetInfo(vir allocation, capacity); - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } @@ -1461,10 +1460,8 @@ virStorageBackendRunProgRegex(virStorage if (list) fclose(list); - else { - if (fd >= 0) - close(fd); - } + else + 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 @@ -45,6 +45,7 @@ #include "util.h" #include "memory.h" #include "xml.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -72,25 +73,25 @@ virStorageBackendProbeTarget(virStorageV if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, allocation, capacity)) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } memset(&meta, 0, sizeof(meta)); if ((target->format = virStorageFileProbeFormatFromFD(target->path, fd)) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } if (virStorageFileGetMetadataFromFD(target->path, fd, target->format, &meta) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } - close(fd); + VIR_FORCE_CLOSE(fd); if (meta.backingStore) { *backingStore = meta.backingStore; @@ -98,7 +99,7 @@ virStorageBackendProbeTarget(virStorageV if (meta.backingStoreFormat == VIR_STORAGE_FILE_AUTO) { if ((*backingStoreFormat = virStorageFileProbeFormat(*backingStore)) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); goto cleanup; } } else { 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 @@ -41,6 +41,7 @@ #include "util.h" #include "memory.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -237,9 +238,7 @@ out: if (fp != NULL) { fclose(fp); } else { - if (fd != -1) { - close(fd); - } + VIR_FORCE_CLOSE(fd); } return ret; Index: libvirt-acl/src/storage/storage_backend_logical.c =================================================================== --- libvirt-acl.orig/src/storage/storage_backend_logical.c +++ libvirt-acl/src/storage/storage_backend_logical.c @@ -37,6 +37,7 @@ #include "util.h" #include "memory.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -408,10 +409,10 @@ virStorageBackendLogicalBuildPool(virCon virReportSystemError(errno, _("cannot clear device header of '%s'"), pool->def->source.devices[i].path); - close(fd); + VIR_FORCE_CLOSE(fd); goto cleanup; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot close device '%s'"), pool->def->source.devices[i].path); @@ -622,7 +623,7 @@ virStorageBackendLogicalCreateVol(virCon goto cleanup; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot close file '%s'"), vol->target.path); @@ -641,8 +642,7 @@ virStorageBackendLogicalCreateVol(virCon return 0; cleanup: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); virStorageBackendLogicalDeleteVol(conn, pool, vol, 0); return -1; } Index: libvirt-acl/src/storage/storage_backend_mpath.c =================================================================== --- libvirt-acl.orig/src/storage/storage_backend_mpath.c +++ libvirt-acl/src/storage/storage_backend_mpath.c @@ -35,6 +35,7 @@ #include "storage_backend.h" #include "memory.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -61,9 +62,7 @@ virStorageBackendMpathUpdateVolTargetInf ret = 0; out: - if (fd != -1) { - close(fd); - } + 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 @@ -32,6 +32,7 @@ #include "storage_backend_scsi.h" #include "memory.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -154,8 +155,7 @@ virStorageBackendSCSIUpdateVolTargetInfo ret = 0; cleanup: - if (fd >= 0) - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } @@ -572,14 +572,14 @@ virStorageBackendSCSITriggerRescan(uint3 if (safewrite(fd, LINUX_SYSFS_SCSI_HOST_SCAN_STRING, sizeof(LINUX_SYSFS_SCSI_HOST_SCAN_STRING)) < 0) { - + VIR_FORCE_CLOSE(fd); virReportSystemError(errno, _("Write to '%s' to trigger host scan failed"), path); retval = -1; } - close(fd); + VIR_FORCE_CLOSE(fd); free_path: VIR_FREE(path); out: Index: libvirt-acl/src/storage/storage_driver.c =================================================================== --- libvirt-acl.orig/src/storage/storage_driver.c +++ libvirt-acl/src/storage/storage_driver.c @@ -45,6 +45,7 @@ #include "memory.h" #include "storage_backend.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1664,9 +1665,7 @@ storageVolumeWipeInternal(virStorageVolD out: VIR_FREE(writebuf); - if (fd != -1) { - close(fd); - } + VIR_FORCE_CLOSE(fd); return ret; } Index: libvirt-acl/src/test/test_driver.c =================================================================== --- libvirt-acl.orig/src/test/test_driver.c +++ libvirt-acl/src/test/test_driver.c @@ -50,6 +50,7 @@ #include "xml.h" #include "threads.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_TEST @@ -788,8 +789,7 @@ static int testOpenFromFile(virConnectPt _("Invalid XML in file '%s'"), file); goto error; } - close(fd); - fd = -1; + VIR_FORCE_CLOSE(fd); root = xmlDocGetRootElement(xml); if ((root == NULL) || (!xmlStrEqual(root->name, BAD_CAST "node"))) { @@ -1101,8 +1101,7 @@ static int testOpenFromFile(virConnectPt VIR_FREE(networks); VIR_FREE(ifaces); VIR_FREE(pools); - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); virDomainObjListDeinit(&privconn->domains); virNetworkObjListFree(&privconn->networks); virInterfaceObjListFree(&privconn->ifaces); @@ -1752,7 +1751,7 @@ static int testDomainSave(virDomainPtr d goto cleanup; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("saving domain '%s' to '%s': write failed"), domain->name, path); @@ -1779,8 +1778,7 @@ cleanup: * in either case we're already in a failure scenario * and have reported a earlier error */ if (ret != 0) { - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); unlink(path); } if (privdom) @@ -1870,8 +1868,7 @@ static int testDomainRestore(virConnectP cleanup: virDomainDefFree(def); VIR_FREE(xml); - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); if (dom) virDomainObjUnlock(dom); if (event) @@ -1911,7 +1908,7 @@ static int testDomainCoreDump(virDomainP domain->name, to); goto cleanup; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("domain '%s' coredump: write failed: %s"), domain->name, to); @@ -1932,8 +1929,7 @@ static int testDomainCoreDump(virDomainP ret = 0; cleanup: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); if (privdom) virDomainObjUnlock(privdom); if (event) Index: libvirt-acl/src/uml/uml_conf.c =================================================================== --- libvirt-acl.orig/src/uml/uml_conf.c +++ libvirt-acl/src/uml/uml_conf.c @@ -47,6 +47,7 @@ #include "bridge.h" #include "logging.h" #include "domain_nwfilter.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_UML @@ -367,7 +368,7 @@ umlBuildCommandLineChr(virDomainChrDefPt } if (virAsprintf(&ret, "%s%d=null,fd:%d", dev, def->target.port, fd_out) < 0) { virReportOOMError(); - close(fd_out); + VIR_FORCE_CLOSE(fd_out); return NULL; } FD_SET(fd_out, keepfd); Index: libvirt-acl/src/uml/uml_driver.c =================================================================== --- libvirt-acl.orig/src/uml/uml_driver.c +++ libvirt-acl/src/uml/uml_driver.c @@ -59,6 +59,7 @@ #include "datatypes.h" #include "logging.h" #include "domain_nwfilter.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_UML @@ -533,7 +534,7 @@ umlShutdown(void) { umlDriverLock(uml_driver); if (uml_driver->inotifyWatch != -1) virEventRemoveHandle(uml_driver->inotifyWatch); - close(uml_driver->inotifyFD); + VIR_FORCE_CLOSE(uml_driver->inotifyFD); virCapabilitiesFree(uml_driver->caps); /* shutdown active VMs @@ -659,8 +660,7 @@ restat: if (bind(priv->monitor, (struct sockaddr *)&addr, sizeof addr) < 0) { virReportSystemError(errno, "%s", _("cannot bind socket")); - close(priv->monitor); - priv->monitor = -1; + VIR_FORCE_CLOSE(priv->monitor); return -1; } @@ -811,7 +811,7 @@ static int umlStartVMDaemon(virConnectPt virDomainObjPtr vm) { const char **argv = NULL, **tmp; const char **progenv = NULL; - int i, ret; + int i, ret, tmpfd; pid_t pid; char *logfile; int logfd = -1; @@ -870,13 +870,13 @@ static int umlStartVMDaemon(virConnectPt if (umlSetCloseExec(logfd) < 0) { virReportSystemError(errno, "%s", _("Unable to set VM logfile close-on-exec flag")); - close(logfd); + VIR_FORCE_CLOSE(logfd); return -1; } if (umlBuildCommandLine(conn, driver, vm, &keepfd, &argv, &progenv) < 0) { - close(logfd); + VIR_FORCE_CLOSE(logfd); virDomainConfVMNWFilterTeardown(vm); umlCleanupTapDevices(conn, vm); return -1; @@ -912,15 +912,17 @@ static int umlStartVMDaemon(virConnectPt -1, &logfd, &logfd, VIR_EXEC_CLEAR_CAPS, NULL, NULL, NULL); - close(logfd); + VIR_FORCE_CLOSE(logfd); /* * At the moment, the only thing that populates keepfd is * umlBuildCommandLineChr. We want to close every fd it opens. */ for (i = 0; i < FD_SETSIZE; i++) - if (FD_ISSET(i, &keepfd)) - close(i); + if (FD_ISSET(i, &keepfd)) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd); + } for (i = 0 ; argv[i] ; i++) VIR_FREE(argv[i]); @@ -957,9 +959,7 @@ static void umlShutdownVMDaemon(virConne virKillProcess(vm->pid, SIGTERM); - if (priv->monitor != -1) - close(priv->monitor); - priv->monitor = -1; + VIR_FORCE_CLOSE(priv->monitor); if ((ret = waitpid(vm->pid, NULL, 0)) != vm->pid) { VIR_WARN("Got unexpected pid %d != %d", Index: libvirt-acl/src/util/bridge.c =================================================================== --- libvirt-acl.orig/src/util/bridge.c +++ libvirt-acl/src/util/bridge.c @@ -24,6 +24,7 @@ #if defined(WITH_BRIDGE) # include "bridge.h" +# include "files.h" # include <stdlib.h> # include <stdio.h> @@ -81,12 +82,12 @@ brInit(brControl **ctlp) if ((flags = fcntl(fd, F_GETFD)) < 0 || fcntl(fd, F_SETFD, flags | FD_CLOEXEC) < 0) { int err = errno; - close(fd); + VIR_FORCE_CLOSE(fd); return err; } if (VIR_ALLOC(*ctlp) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); return ENOMEM; } @@ -107,7 +108,7 @@ brShutdown(brControl *ctl) if (!ctl) return; - close(ctl->fd); + VIR_FORCE_CLOSE(ctl->fd); ctl->fd = 0; VIR_FREE(ctl); @@ -539,11 +540,11 @@ brAddTap(brControl *ctl, if (tapfd) *tapfd = fd; else - close(fd); + VIR_FORCE_CLOSE(fd); return 0; error: - close(fd); + VIR_FORCE_CLOSE(fd); return errno; } @@ -574,7 +575,7 @@ int brDeleteTap(brControl *ctl, } error: - close(fd); + VIR_FORCE_CLOSE(fd); return errno; } Index: libvirt-acl/src/util/conf.c =================================================================== --- libvirt-acl.orig/src/util/conf.c +++ libvirt-acl/src/util/conf.c @@ -24,6 +24,7 @@ #include "util.h" #include "c-ctype.h" #include "memory.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_CONF @@ -954,7 +955,7 @@ virConfWriteFile(const char *filename, v content = virBufferContentAndReset(&buf); ret = safewrite(fd, content, use); VIR_FREE(content); - close(fd); + VIR_FORCE_CLOSE(fd); if (ret != (int)use) { virConfError(NULL, VIR_ERR_WRITE_FAILED, _("failed to save content")); return -1; Index: libvirt-acl/src/util/interface.c =================================================================== --- libvirt-acl.orig/src/util/interface.c +++ libvirt-acl/src/util/interface.c @@ -39,6 +39,7 @@ #include "util.h" #include "interface.h" #include "virterror_internal.h" +#include "files.h" #define ifaceError(code, ...) \ virReportErrorHelper(NULL, VIR_FROM_NET, code, __FILE__, \ @@ -82,7 +83,7 @@ ifaceGetFlags(const char *ifname, short *flags = ifr.ifr_flags; - close(fd); + VIR_FORCE_CLOSE(fd); return rc; } @@ -161,7 +162,7 @@ static int chgIfaceFlags(const char *ifn } err_exit: - close(fd); + VIR_FORCE_CLOSE(fd); return rc; } @@ -259,8 +260,7 @@ ifaceCheck(bool reportError, const char } err_exit: - if (fd >= 0) - close(fd); + VIR_FORCE_CLOSE(fd); return rc; } @@ -326,7 +326,7 @@ ifaceGetIndex(bool reportError, const ch } err_exit: - close(fd); + VIR_FORCE_CLOSE(fd); return rc; } @@ -373,7 +373,7 @@ ifaceGetVlanID(const char *vlanifname, i *vlanid = vlanargs.u.VID; err_exit: - close(fd); + VIR_FORCE_CLOSE(fd); return rc; } Index: libvirt-acl/src/util/logging.c =================================================================== --- libvirt-acl.orig/src/util/logging.c +++ libvirt-acl/src/util/logging.c @@ -40,6 +40,7 @@ #include "util.h" #include "buf.h" #include "threads.h" +#include "files.h" /* * Macro used to format the message as a string in virLogMessage @@ -603,8 +604,7 @@ static int virLogOutputToFd(const char * static void virLogCloseFd(void *data) { int fd = (long) data; - if (fd >= 0) - close(fd); + VIR_FORCE_CLOSE(fd); } static int virLogAddOutputToStderr(int priority) { @@ -622,7 +622,7 @@ static int virLogAddOutputToFile(int pri return(-1); if (virLogDefineOutput(virLogOutputToFd, virLogCloseFd, (void *)(long)fd, priority, VIR_LOG_TO_FILE, file, 0) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); return(-1); } return(0); Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -52,6 +52,7 @@ # include "conf/domain_conf.h" # include "virterror_internal.h" # include "uuid.h" +# include "files.h" # define VIR_FROM_THIS VIR_FROM_NET @@ -94,7 +95,7 @@ static int nlOpen(void) static void nlClose(int fd) { - close(fd); + VIR_FORCE_CLOSE(fd); } @@ -689,7 +690,7 @@ create_name: if (rc >= 0) { if (configMacvtapTap(rc, vnet_hdr) < 0) { - close(rc); + VIR_FORCE_CLOSE(rc); rc = -1; goto disassociate_exit; } @@ -778,8 +779,7 @@ getLldpadPid(void) { _("Error opening file %s"), LLDPAD_PID_FILE); } - if (fd >= 0) - close(fd); + VIR_FORCE_CLOSE(fd); return pid; } Index: libvirt-acl/src/util/pci.c =================================================================== --- libvirt-acl.orig/src/util/pci.c +++ libvirt-acl/src/util/pci.c @@ -37,6 +37,7 @@ #include "memory.h" #include "util.h" #include "virterror_internal.h" +#include "files.h" /* avoid compilation breakage on some systems */ #ifndef MODPROBE @@ -188,10 +189,7 @@ pciCloseConfig(pciDevice *dev) if (!dev) return; - if (dev->fd >= 0) { - close(dev->fd); - dev->fd = -1; - } + VIR_FORCE_CLOSE(dev->fd); } static int Index: libvirt-acl/src/util/storage_file.c =================================================================== --- libvirt-acl.orig/src/util/storage_file.c +++ libvirt-acl/src/util/storage_file.c @@ -36,6 +36,7 @@ #include "memory.h" #include "virterror_internal.h" #include "logging.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -688,7 +689,7 @@ virStorageFileProbeFormat(const char *pa ret = virStorageFileProbeFormatFromFD(path, fd); - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } @@ -782,7 +783,7 @@ virStorageFileGetMetadata(const char *pa ret = virStorageFileGetMetadataFromFD(path, fd, format, meta); - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } Index: libvirt-acl/src/util/util.c =================================================================== --- libvirt-acl.orig/src/util/util.c +++ libvirt-acl/src/util/util.c @@ -71,6 +71,7 @@ #include "memory.h" #include "threads.h" #include "verify.h" +#include "files.h" #ifndef NSIG # define NSIG 32 @@ -461,6 +462,7 @@ __virExec(const char *const*argv, int pipeerr[2] = {-1,-1}; int childout = -1; int childerr = -1; + int tmpfd; if ((null = open("/dev/null", O_RDONLY)) < 0) { virReportSystemError(errno, @@ -534,13 +536,13 @@ __virExec(const char *const*argv, } if (pid) { /* parent */ - close(null); + VIR_FORCE_CLOSE(null); if (outfd && *outfd == -1) { - close(pipeout[1]); + VIR_FORCE_CLOSE(pipeout[1]); *outfd = pipeout[0]; } if (errfd && *errfd == -1) { - close(pipeerr[1]); + VIR_FORCE_CLOSE(pipeerr[1]); *errfd = pipeerr[0]; } @@ -568,8 +570,10 @@ __virExec(const char *const*argv, i != childout && i != childerr && (!keepfd || - !FD_ISSET(i, keepfd))) - close(i); + !FD_ISSET(i, keepfd))) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd); + } if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) { virReportSystemError(errno, @@ -589,14 +593,15 @@ __virExec(const char *const*argv, goto fork_error; } - if (infd > 0) - close(infd); - close(null); - if (childout > 0) - close(childout); + VIR_FORCE_CLOSE(infd); + VIR_FORCE_CLOSE(null); + tmpfd = childout; /* preserve childout value */ + VIR_FORCE_CLOSE(tmpfd); if (childerr > 0 && - childerr != childout) - close(childerr); + childerr != childout) { + VIR_FORCE_CLOSE(childerr); + childout = -1; + } /* Daemonize as late as possible, so the parent process can detect * the above errors with wait* */ @@ -666,16 +671,11 @@ __virExec(const char *const*argv, /* NB we don't virUtilError() on any failures here because the code which jumped hre already raised an error condition which we must not overwrite */ - if (pipeerr[0] > 0) - close(pipeerr[0]); - if (pipeerr[1] > 0) - close(pipeerr[1]); - if (pipeout[0] > 0) - close(pipeout[0]); - if (pipeout[1] > 0) - close(pipeout[1]); - if (null > 0) - close(null); + VIR_FORCE_CLOSE(pipeerr[0]); + VIR_FORCE_CLOSE(pipeerr[1]); + VIR_FORCE_CLOSE(pipeout[0]); + VIR_FORCE_CLOSE(pipeout[1]); + VIR_FORCE_CLOSE(null); return -1; } @@ -865,10 +865,8 @@ virRunWithHook(const char *const*argv, VIR_FREE(outbuf); VIR_FREE(errbuf); VIR_FREE(argv_str); - if (outfd != -1) - close(outfd); - if (errfd != -1) - close(errfd); + VIR_FORCE_CLOSE(outfd); + VIR_FORCE_CLOSE(errfd); return ret; } @@ -1099,7 +1097,7 @@ int virFileReadAll(const char *path, int } int len = virFileReadLimFD(fd, maxlen, buf); - close(fd); + VIR_FORCE_CLOSE(fd); if (len < 0) { virReportSystemError(errno, _("Failed to read file '%s'"), path); return -1; @@ -1305,7 +1303,7 @@ static int virFileOperationNoFork(const if ((hook) && ((ret = hook(fd, hookdata)) != 0)) { goto error; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { ret = -errno; virReportSystemError(errno, _("failed to close new file '%s'"), path); @@ -1314,8 +1312,7 @@ static int virFileOperationNoFork(const } fd = -1; error: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } @@ -1466,7 +1463,7 @@ parenterror: if ((hook) && ((ret = hook(fd, hookdata)) != 0)) { goto childerror; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { ret = -errno; virReportSystemError(errno, _("child failed to close new file '%s'"), path); @@ -1743,10 +1740,8 @@ int virFileOpenTtyAt(const char *ptmx, rc = 0; cleanup: - if (rc != 0 && - *ttymaster != -1) { - close(*ttymaster); - } + if (rc != 0) + VIR_FORCE_CLOSE(*ttymaster); return rc; @@ -1812,7 +1807,7 @@ int virFileWritePidPath(const char *pidf if (!(file = fdopen(fd, "w"))) { rc = errno; - close(fd); + VIR_FORCE_CLOSE(fd); goto cleanup; } Index: libvirt-acl/src/util/uuid.c =================================================================== --- libvirt-acl.orig/src/util/uuid.c +++ libvirt-acl/src/util/uuid.c @@ -39,6 +39,7 @@ #include "virterror_internal.h" #include "logging.h" #include "memory.h" +#include "files.h" #ifndef ENODATA # define ENODATA EIO @@ -61,7 +62,7 @@ virUUIDGenerateRandomBytes(unsigned char if ((n = read(fd, buf, buflen)) <= 0) { if (errno == EINTR) continue; - close(fd); + VIR_FORCE_CLOSE(fd); return n < 0 ? errno : ENODATA; } @@ -69,7 +70,7 @@ virUUIDGenerateRandomBytes(unsigned char buflen -= n; } - close(fd); + VIR_FORCE_CLOSE(fd); return 0; } @@ -240,10 +241,10 @@ getDMISystemUUID(char *uuid, int len) int fd = open(paths[i], O_RDONLY); if (fd > 0) { if (saferead(fd, uuid, len) == len) { - close(fd); + VIR_FORCE_CLOSE(fd); return 0; } - close(fd); + VIR_FORCE_CLOSE(fd); } i++; } Index: libvirt-acl/src/util/virtaudit.c =================================================================== --- libvirt-acl.orig/src/util/virtaudit.c +++ libvirt-acl/src/util/virtaudit.c @@ -30,6 +30,7 @@ #include "virterror_internal.h" #include "logging.h" #include "virtaudit.h" +#include "files.h" /* Provide the macros in case the header file is old. FIXME: should be removed. */ @@ -133,6 +134,6 @@ void virAuditSend(const char *file ATTRI void virAuditClose(void) { #if HAVE_AUDIT - close(auditfd); + VIR_CLOSE(auditfd); #endif } Index: libvirt-acl/src/xen/proxy_internal.c =================================================================== --- libvirt-acl.orig/src/xen/proxy_internal.c +++ libvirt-acl/src/xen/proxy_internal.c @@ -30,6 +30,7 @@ #include "util.h" #include "xen_driver.h" #include "memory.h" +#include "files.h" #define STANDALONE @@ -196,7 +197,7 @@ retry: addr.sun_family = AF_UNIX; addr.sun_path[0] = '\0'; if (virStrcpy(&addr.sun_path[1], path, sizeof(addr.sun_path) - 1) == NULL) { - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -204,7 +205,7 @@ retry: * now bind the socket to that address and listen on it */ if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { - close(fd); + VIR_FORCE_CLOSE(fd); if (trials < 3) { if (virProxyForkServer() < 0) return(-1); @@ -236,12 +237,11 @@ virProxyCloseSocket(xenUnifiedPrivatePtr if (priv->proxy < 0) return(-1); - ret = close(priv->proxy); + ret = VIR_CLOSE(priv->proxy); if (ret != 0) VIR_WARN("Failed to close socket %d", priv->proxy); else VIR_DEBUG("Closed socket %d", priv->proxy); - priv->proxy = -1; return(ret); } Index: libvirt-acl/src/xen/xen_hypervisor.c =================================================================== --- libvirt-acl.orig/src/xen/xen_hypervisor.c +++ libvirt-acl/src/xen/xen_hypervisor.c @@ -65,6 +65,7 @@ #include "buf.h" #include "capabilities.h" #include "memory.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_XEN @@ -2036,7 +2037,7 @@ xenHypervisorInit(void) hypervisor_version = -1; virXenError(VIR_ERR_XEN_CALL, " ioctl %lu", (unsigned long) IOCTL_PRIVCMD_HYPERCALL); - close(fd); + VIR_FORCE_CLOSE(fd); in_init = 0; return(-1); @@ -2122,13 +2123,13 @@ xenHypervisorInit(void) hypervisor_version = -1; virXenError(VIR_ERR_XEN_CALL, " ioctl %lu", (unsigned long)IOCTL_PRIVCMD_HYPERCALL); - close(fd); + VIR_FORCE_CLOSE(fd); in_init = 0; VIR_FREE(ipt); return(-1); done: - close(fd); + VIR_FORCE_CLOSE(fd); in_init = 0; VIR_FREE(ipt); return(0); @@ -2191,7 +2192,7 @@ xenHypervisorClose(virConnectPtr conn) if (priv->handle < 0) return -1; - ret = close(priv->handle); + ret = VIR_CLOSE(priv->handle); if (ret < 0) return (-1); @@ -2396,8 +2397,7 @@ get_cpu_flags(virConnectPtr conn, const ret = 1; out: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); return ret; } Index: libvirt-acl/src/xen/xen_inotify.c =================================================================== --- libvirt-acl.orig/src/xen/xen_inotify.c +++ libvirt-acl/src/xen/xen_inotify.c @@ -39,6 +39,7 @@ #include "xend_internal.h" #include "logging.h" #include "uuid.h" +#include "files.h" #include "xm_internal.h" /* for xenXMDomainConfigParse */ @@ -483,7 +484,7 @@ xenInotifyClose(virConnectPtr conn) if (priv->inotifyWatch != -1) virEventRemoveHandle(priv->inotifyWatch); - close(priv->inotifyFD); + VIR_FORCE_CLOSE(priv->inotifyFD); return 0; } Index: libvirt-acl/src/xen/xend_internal.c =================================================================== --- libvirt-acl.orig/src/xen/xend_internal.c +++ libvirt-acl/src/xen/xend_internal.c @@ -45,6 +45,7 @@ #include "xs_internal.h" /* To extract VNC port & Serial console TTY */ #include "memory.h" #include "count-one-bits.h" +#include "files.h" /* required for cpumap_t */ #include <xen/dom0_ops.h> @@ -118,7 +119,6 @@ static int do_connect(virConnectPtr xend) { int s; - int serrno; int no_slow_start = 1; xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) xend->privateData; @@ -137,9 +137,7 @@ do_connect(virConnectPtr xend) if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) == -1) { - serrno = errno; - close(s); - errno = serrno; + VIR_FORCE_CLOSE(s); s = -1; /* @@ -387,7 +385,7 @@ xend_get(virConnectPtr xend, const char "Content-Type: application/x-www-form-urlencoded\r\n" "\r\n"); ret = xend_req(s, content); - close(s); + VIR_FORCE_CLOSE(s); if (((ret < 0) || (ret >= 300)) && ((ret != 404) || (!STRPREFIX(path, "/xend/domain/")))) { @@ -437,7 +435,7 @@ xend_post(virConnectPtr xend, const char swrites(s, ops); ret = xend_req(s, &err_buf); - close(s); + VIR_FORCE_CLOSE(s); if ((ret < 0) || (ret >= 300)) { virXendError(VIR_ERR_POST_FAILED, @@ -843,7 +841,7 @@ xenDaemonOpen_tcp(virConnectPtr conn, co memcpy(&priv->addr, r->ai_addr, r->ai_addrlen); - close(sock); + VIR_FORCE_CLOSE(sock); break; } Index: libvirt-acl/daemon/libvirtd.c =================================================================== --- libvirt-acl.orig/daemon/libvirtd.c +++ libvirt-acl/daemon/libvirtd.c @@ -50,6 +50,7 @@ #include "libvirt_internal.h" #include "virterror_internal.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -425,7 +426,7 @@ static int daemonForkIntoBackground(void int stdoutfd = -1; int nextpid; - close(statuspipe[0]); + VIR_FORCE_CLOSE(statuspipe[0]); if ((stdinfd = open("/dev/null", O_RDONLY)) < 0) goto cleanup; @@ -437,12 +438,10 @@ static int daemonForkIntoBackground(void goto cleanup; if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) goto cleanup; - if (close(stdinfd) < 0) + if (VIR_CLOSE(stdinfd) < 0) goto cleanup; - stdinfd = -1; - if (close(stdoutfd) < 0) + if (VIR_CLOSE(stdoutfd) < 0) goto cleanup; - stdoutfd = -1; if (setsid() < 0) goto cleanup; @@ -458,10 +457,8 @@ static int daemonForkIntoBackground(void } cleanup: - if (stdoutfd != -1) - close(stdoutfd); - if (stdinfd != -1) - close(stdinfd); + VIR_FORCE_CLOSE(stdoutfd); + VIR_FORCE_CLOSE(stdinfd); return -1; } @@ -475,7 +472,7 @@ static int daemonForkIntoBackground(void int ret; char status; - close(statuspipe[1]); + VIR_FORCE_CLOSE(statuspipe[1]); /* We wait to make sure the first child forked successfully */ if ((got = waitpid(pid, &exitstatus, 0)) < 0 || @@ -518,7 +515,7 @@ static int qemudWritePidFile(const char if (!(fh = fdopen(fd, "w"))) { VIR_ERROR(_("Failed to fdopen pid file '%s' : %s"), pidFile, virStrerror(errno, ebuf, sizeof ebuf)); - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -607,8 +604,7 @@ static int qemudListenUnix(struct qemud_ return 0; cleanup: - if (sock->fd >= 0) - close(sock->fd); + VIR_FORCE_CLOSE(sock->fd); VIR_FREE(sock); return -1; } @@ -745,7 +741,7 @@ remoteListenTCP (struct qemud_server *se cleanup: for (i = 0; i < nfds; ++i) - close(fds[i]); + VIR_FORCE_CLOSE(fds[i]); return -1; } @@ -1518,10 +1514,7 @@ void qemudDispatchClientFailure(struct q gnutls_deinit (client->tlssession); client->tlssession = NULL; } - if (client->fd != -1) { - close(client->fd); - client->fd = -1; - } + VIR_FORCE_CLOSE(client->fd); } @@ -2421,17 +2414,15 @@ static int qemudStartEventLoop(struct qe static void qemudCleanup(struct qemud_server *server) { struct qemud_socket *sock; - if (server->sigread != -1) - close(server->sigread); - if (server->sigwrite != -1) - close(server->sigwrite); + VIR_FORCE_CLOSE(server->sigread); + VIR_FORCE_CLOSE(server->sigwrite); sock = server->sockets; while (sock) { struct qemud_socket *next = sock->next; if (sock->watch) virEventRemoveHandleImpl(sock->watch); - close(sock->fd); + VIR_FORCE_CLOSE(sock->fd); /* Unlink unix domain sockets which are not in * the abstract namespace */ @@ -2986,8 +2977,8 @@ daemonSetupSignals(struct qemud_server * return 0; error: - close(sigpipe[0]); - close(sigpipe[1]); + VIR_FORCE_CLOSE(sigpipe[0]); + VIR_FORCE_CLOSE(sigpipe[1]); return -1; } @@ -3244,8 +3235,7 @@ int main(int argc, char **argv) { while (write(statuswrite, &status, 1) == -1 && errno == EINTR) ; - close(statuswrite); - statuswrite = -1; + VIR_FORCE_CLOSE(statuswrite); } /* Start the event loop in a background thread, since @@ -3302,7 +3292,7 @@ error: errno == EINTR) ; } - close(statuswrite); + VIR_FORCE_CLOSE(statuswrite); } if (server) qemudCleanup(server); Index: libvirt-acl/src/util/hooks.c =================================================================== --- libvirt-acl.orig/src/util/hooks.c +++ libvirt-acl/src/util/hooks.c @@ -36,6 +36,7 @@ #include "conf/domain_conf.h" #include "logging.h" #include "memory.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_HOOK @@ -368,7 +369,7 @@ virHookCall(int driver, const char *id, } ret = virExec(argv, env, NULL, &pid, pipefd[0], &outfd, &errfd, VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); - if (close(pipefd[1]) < 0) { + if (VIR_CLOSE(pipefd[1]) < 0) { virReportSystemError(errno, "%s", _("unable to close pipe for hook input")); } @@ -418,17 +419,13 @@ virHookCall(int driver, const char *id, } cleanup: - if (pipefd[0] >= 0) { - if (close(pipefd[0]) < 0) { - virReportSystemError(errno, "%s", - _("unable to close pipe for hook input")); - } - } - if (pipefd[1] >= 0) { - if (close(pipefd[1]) < 0) { - virReportSystemError(errno, "%s", - _("unable to close pipe for hook input")); - } + if (VIR_CLOSE(pipefd[0]) < 0) { + virReportSystemError(errno, "%s", + _("unable to close pipe for hook input")); + } + if (VIR_CLOSE(pipefd[1]) < 0) { + virReportSystemError(errno, "%s", + _("unable to close pipe for hook input")); } if (argv) { for (i = 0 ; i < argc ; i++) Index: libvirt-acl/tests/testutils.c =================================================================== --- libvirt-acl.orig/tests/testutils.c +++ libvirt-acl/tests/testutils.c @@ -47,6 +47,8 @@ ((((int) ((T)->tv_sec - (U)->tv_sec)) * 1000000.0 + \ ((int) ((T)->tv_usec - (U)->tv_usec))) / 1000.0) +#include "files.h" + static unsigned int testDebug = -1; static unsigned int testVerbose = -1; @@ -205,7 +207,7 @@ int virtTestLoadFile(const char *file, static void virtTestCaptureProgramExecChild(const char *const argv[], int pipefd) { - int i; + int i, tmpfd; int open_max; int stdinfd = -1; const char *const env[] = { @@ -222,8 +224,10 @@ void virtTestCaptureProgramExecChild(con open_max = sysconf (_SC_OPEN_MAX); for (i = 0; i < open_max; i++) { if (i != stdinfd && - i != pipefd) - close(i); + i != pipefd) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd); + } } if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) @@ -237,8 +241,7 @@ void virtTestCaptureProgramExecChild(con execve(argv[0], (char *const*)argv, (char *const*)env); cleanup: - if (stdinfd != -1) - close(stdinfd); + VIR_FORCE_CLOSE(stdinfd); } int virtTestCaptureProgramOutput(const char *const argv[], @@ -252,10 +255,10 @@ int virtTestCaptureProgramOutput(const c int pid = fork(); switch (pid) { case 0: - close(pipefd[0]); + VIR_FORCE_CLOSE(pipefd[0]); virtTestCaptureProgramExecChild(argv, pipefd[1]); - close(pipefd[1]); + VIR_FORCE_CLOSE(pipefd[1]); _exit(1); case -1: @@ -267,7 +270,7 @@ int virtTestCaptureProgramOutput(const c int ret = -1; int want = buflen-1; - close(pipefd[1]); + VIR_FORCE_CLOSE(pipefd[1]); while (want) { if ((ret = read(pipefd[0], (*buf)+got, want)) <= 0) @@ -275,7 +278,7 @@ int virtTestCaptureProgramOutput(const c got += ret; want -= ret; } - close(pipefd[0]); + VIR_FORCE_CLOSE(pipefd[0]); if (!ret) (*buf)[got] = '\0'; Index: libvirt-acl/proxy/libvirt_proxy.c =================================================================== --- libvirt-acl.orig/proxy/libvirt_proxy.c +++ libvirt-acl/proxy/libvirt_proxy.c @@ -31,6 +31,7 @@ # include "xend_internal.h" # include "xs_internal.h" # include "xen_driver.h" +# include "files.h" static int fdServer = -1; static int debug = 0; @@ -133,10 +134,9 @@ proxyCloseUnixSocket(void) { if (fdServer < 0) return(0); - ret = close(fdServer); if (debug > 0) fprintf(stderr, "closing unix socket %d: %d\n", fdServer, ret); - fdServer = -1; + ret = VIR_CLOSE(fdServer); pollInfos[0].fd = -1; return(ret); } @@ -172,7 +172,7 @@ proxyListenUnixSocket(const char *path) addr.sun_path[0] = '\0'; if (virStrcpy(&addr.sun_path[1], path, sizeof(addr.sun_path) - 1) == NULL) { fprintf(stderr, "Path %s too long to fit into destination\n", path); - close(fd); + VIR_FORCE_CLOSE(fd); return -1; } @@ -181,12 +181,12 @@ proxyListenUnixSocket(const char *path) */ if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { fprintf(stderr, "Failed to bind to socket %s\n", path); - close(fd); + VIR_FORCE_CLOSE(fd); return (-1); } if (listen(fd, 30 /* backlog */ ) < 0) { fprintf(stderr, "Failed to listen to socket %s\n", path); - close(fd); + VIR_FORCE_CLOSE(fd); return (-1); } @@ -230,7 +230,7 @@ retry: if (nbClients >= MAX_CLIENT) { fprintf(stderr, "Too many client registered\n"); - close(client); + VIR_FORCE_CLOSE(client); return(-1); } nbClients++; @@ -260,7 +260,7 @@ static int proxyCloseClientSocket(int nr) { int ret; - ret = close(pollInfos[nr].fd); + ret = VIR_CLOSE(pollInfos[nr].fd); if (ret != 0) fprintf(stderr, "Failed to close socket %d from client %d\n", pollInfos[nr].fd, nr); @@ -285,7 +285,7 @@ proxyCloseClientSockets(void) { int i, ret; for (i = 1;i <= nbClients;i++) { - ret = close(pollInfos[i].fd); + ret = VIR_CLOSE(pollInfos[i].fd); if (ret != 0) fprintf(stderr, "Failed to close socket %d from client %d\n", pollInfos[i].fd, i); Index: libvirt-acl/tools/console.c =================================================================== --- libvirt-acl.orig/tools/console.c +++ libvirt-acl/tools/console.c @@ -39,6 +39,7 @@ # include "internal.h" # include "logging.h" # include "util.h" +# include "files.h" /* ie Ctrl-] as per telnet */ # define CTRL_CLOSE_BRACKET '\35' @@ -192,7 +193,7 @@ int vshRunConsole(const char *tty) { tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); closetty: - close(ttyfd); + VIR_FORCE_CLOSE(ttyfd); return ret; }
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@us.ibm.com>
Pre-review comment: One more thing to do, which is adding something like this to cfg.mk: sc_avoid_close: @prohibit='\<close *\(' \ in_vc_files='\.c$$' \ halt='use VIR_(FORCE_)CLOSE instead of close' \ $(_sc_search_regexp) along with adding .x-sc_avoid_close with contents of src/util/file.c to exempt the one valid use of close(). Would you mind testing 'make syntax-check' after folding that in, and addressing any other fallout that it detects, while I proceed with the rest of my review? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
libvir-list-bounces@redhat.com wrote on 10/22/2010 12:33:48 PM:
On 10/22/2010 05:19 AM, Stefan Berger wrote:
Using automated replacement with sed and editing I have now replaced
occurrences of close() with VIR_(FORCE_)CLOSE() except for one, of course. Some replacements were straight forward, others I needed to
all 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@us.ibm.com>
Pre-review comment: One more thing to do, which is adding something like this to cfg.mk:
sc_avoid_close: @prohibit='\<close *\(' \ in_vc_files='\.c$$' \ halt='use VIR_(FORCE_)CLOSE instead of close' \ $(_sc_search_regexp)
along with adding .x-sc_avoid_close with contents of src/util/file.c to exempt the one valid use of close().
Would you mind testing 'make syntax-check' after folding that in, and addressing any other fallout that it detects, while I proceed with the rest of my review?
Yes, I can do that... runs fine. The patch will need to be rebased to tip... Stefan
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@us.ibm.com>
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@us.ibm.com>
--- daemon/libvirtd.c | 46 ++++++---------
Continuing on (looks like I'll be replying quite a few times today)...
@@ -127,7 +128,7 @@ static int lxcContainerExecInit(virDomai static int lxcContainerSetStdio(int control, int ttyfd) { int rc = -1; - int open_max, i; + int open_max, i, tpmfd;
if (setsid()< 0) { virReportSystemError(errno, "%s", @@ -145,8 +146,10 @@ static int lxcContainerSetStdio(int cont * close all FDs before executing the container */ open_max = sysconf (_SC_OPEN_MAX); for (i = 0; i< open_max; i++) - if (i != ttyfd&& i != control) - close(i); + if (i != ttyfd&& i != control) { + tpmfd = i; + VIR_FORCE_CLOSE(tpmfd);
Yeah, I guess you do have to introduce a temporary rather than clobbering your iterator. s/tpmfd/tmpfd/, and perhaps reduce it's scope to just the for loop or if statement where it is needed.
Index: libvirt-acl/src/lxc/lxc_driver.c =================================================================== --- libvirt-acl.orig/src/lxc/lxc_driver.c +++ libvirt-acl/src/lxc/lxc_driver.c @@ -1544,14 +1544,10 @@ cleanup: vethDelete(veths[i]); VIR_FREE(veths[i]); } - if (rc != 0&& priv->monitor != -1) { - close(priv->monitor); - priv->monitor = -1; - } - if (parentTty != -1) - close(parentTty); - if (logfd != -1) - close(logfd); + if (rc != 0) + VIR_FORCE_CLOSE(priv->monitor); + VIR_FORCE_CLOSE(parentTty); + VIR_FORCE_CLOSE(logfd);
logfd might be one where we want to hoist a normal VIR_CLOSE and check for errors.
Index: libvirt-acl/src/phyp/phyp_driver.c
Pausing here for my lunch... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
libvir-list-bounces@redhat.com wrote on 10/22/2010 02:24:38 PM: 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@us.ibm.com>
--- daemon/libvirtd.c | 46 ++++++---------
Continuing on (looks like I'll be replying quite a few times today)...
@@ -127,7 +128,7 @@ static int lxcContainerExecInit(virDomai static int lxcContainerSetStdio(int control, int ttyfd) { int rc = -1; - int open_max, i; + int open_max, i, tpmfd;
if (setsid()< 0) { virReportSystemError(errno, "%s", @@ -145,8 +146,10 @@ static int lxcContainerSetStdio(int cont * close all FDs before executing the container */ open_max = sysconf (_SC_OPEN_MAX); for (i = 0; i< open_max; i++) - if (i != ttyfd&& i != control) - close(i); + if (i != ttyfd&& i != control) { + tpmfd = i; + VIR_FORCE_CLOSE(tpmfd);
Yeah, I guess you do have to introduce a temporary rather than clobbering your iterator.
s/tpmfd/tmpfd/, and perhaps reduce it's scope to just the for loop or if
statement where it is needed.
ha, what a typo... making it a local variable.
- close(logfd); + if (rc != 0) + VIR_FORCE_CLOSE(priv->monitor); + VIR_FORCE_CLOSE(parentTty); + VIR_FORCE_CLOSE(logfd);
logfd might be one where we want to hoist a normal VIR_CLOSE and check for errors.
Ok, so I call the virReportSystemError() on this now. Stefan
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.
Can you isolate any of those double-close errors into separate patches which we can apply now, rather than drowning them in the giant patch?
Signed-off-by: Stefan Berger<stefanb@us.ibm.com>
src/phyp/phyp_driver.c | 13 ++--
Resuming... Most of the changes are looking okay in limited context. The point was raised on IRC that this patch is big enough that we probably ought to delay until post 0.8.5 to apply it, so as to maximize testing exposure over the full course of a release cycle rather than making this next week be all the testing we give. Exceptions for true double-close bug fixes which can be applied as independent patches now.
Index: libvirt-acl/src/qemu/qemu_driver.c @@ -6542,8 +6536,10 @@ static int qemudDomainSaveImageClose(int { int ret = 0;
- if (fd != -1) - close(fd); + if (VIR_CLOSE(fd)< 0) { + virReportSystemError(errno, "%s", + _("cannot close file")); + }
Yep, this looks like a good case to convert over to reporting an error.
Index: libvirt-acl/src/qemu/qemu_monitor.c
@@ -694,8 +695,7 @@ void qemuMonitorClose(qemuMonitorPtr mon if (!mon->closed) { if (mon->watch) virEventRemoveHandle(mon->watch); - if (mon->fd != -1) - close(mon->fd); + VIR_FORCE_CLOSE(mon->fd); /* NB: ordinarily one might immediately set mon->watch to -1 * and mon->fd to -1, but there may be a callback active * that is still relying on these fields being valid. So
Ouch - given that comment, could we be frying a callback by setting mon->fd to -1 in VIR_FORCE_CLOSE? We need to double check this, and possibly use a temporary variable if the callback indeed needs a non-negative mon->fd for a bit longer, or tighten the specification and all existing callbacks to tolerate mon->fd changing to -1. Probably worth splitting this particular hunk into its own commit, rather than part of the giant patch.
Index: libvirt-acl/src/remote/remote_driver.c =================================================================== --- libvirt-acl.orig/src/remote/remote_driver.c +++ libvirt-acl/src/remote/remote_driver.c @@ -82,6 +82,7 @@ #include "util.h" #include "event.h" #include "ignore-value.h" +#include "files.h"
#define VIR_FROM_THIS VIR_FROM_REMOTE
@@ -711,7 +712,7 @@ doRemoteOpen (virConnectPtr conn, if (errno == ECONNREFUSED&& flags& VIR_DRV_OPEN_REMOTE_AUTOSTART&& trials< 20) { - close(priv->sock); + VIR_FORCE_CLOSE(priv->sock); priv->sock = -1;
This line is now redundant.
Index: libvirt-acl/src/uml/uml_driver.c @@ -811,7 +811,7 @@ static int umlStartVMDaemon(virConnectPt virDomainObjPtr vm) { const char **argv = NULL, **tmp; const char **progenv = NULL; - int i, ret; + int i, ret, tmpfd; pid_t pid; char *logfile; int logfd = -1;
for (i = 0; i< FD_SETSIZE; i++) - if (FD_ISSET(i,&keepfd)) - close(i); + if (FD_ISSET(i,&keepfd)) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd);
Another awfully large scope for a needed temporary.
Index: libvirt-acl/src/util/bridge.c @@ -107,7 +108,7 @@ brShutdown(brControl *ctl) if (!ctl) return;
- close(ctl->fd); + VIR_FORCE_CLOSE(ctl->fd); ctl->fd = 0;
Huh - is this an existing logic bug? Can we end up accidentally double-closing stdin?
Index: libvirt-acl/src/util/logging.c =================================================================== --- libvirt-acl.orig/src/util/logging.c +++ libvirt-acl/src/util/logging.c @@ -40,6 +40,7 @@ #include "util.h" #include "buf.h" #include "threads.h" +#include "files.h"
/* * Macro used to format the message as a string in virLogMessage @@ -603,8 +604,7 @@ static int virLogOutputToFd(const char * static void virLogCloseFd(void *data) { int fd = (long) data;
- if (fd>= 0) - close(fd); + VIR_FORCE_CLOSE(fd);
Should we fix this function to return an int value, and return VIR_CLOSE(fd) so that callers can choose to detect log close failures?
Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -52,6 +52,7 @@ # include "conf/domain_conf.h" # include "virterror_internal.h" # include "uuid.h" +# include "files.h"
# define VIR_FROM_THIS VIR_FROM_NET
@@ -94,7 +95,7 @@ static int nlOpen(void)
static void nlClose(int fd) { - close(fd); + VIR_FORCE_CLOSE(fd);
Likewise?
Index: libvirt-acl/src/util/util.c =================================================================== --- libvirt-acl.orig/src/util/util.c +++ libvirt-acl/src/util/util.c @@ -71,6 +71,7 @@ #include "memory.h" #include "threads.h" #include "verify.h" +#include "files.h"
#ifndef NSIG # define NSIG 32 @@ -461,6 +462,7 @@ __virExec(const char *const*argv, int pipeerr[2] = {-1,-1}; int childout = -1; int childerr = -1; + int tmpfd;
@@ -568,8 +570,10 @@ __virExec(const char *const*argv, i != childout&& i != childerr&& (!keepfd || - !FD_ISSET(i, keepfd))) - close(i); + !FD_ISSET(i, keepfd))) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd);
I thought this was another large scope for a temporary....
+ }
if (dup2(infd>= 0 ? infd : null, STDIN_FILENO)< 0) { virReportSystemError(errno, @@ -589,14 +593,15 @@ __virExec(const char *const*argv, goto fork_error; }
- if (infd> 0) - close(infd); - close(null); - if (childout> 0) - close(childout); + VIR_FORCE_CLOSE(infd); + VIR_FORCE_CLOSE(null); + tmpfd = childout; /* preserve childout value */ + VIR_FORCE_CLOSE(tmpfd);
...but you needed it here too.
if (childerr> 0&& - childerr != childout) - close(childerr); + childerr != childout) { + VIR_FORCE_CLOSE(childerr); + childout = -1; + }
Looks okay after all - certainly not one of the trivial conversions though :)
Index: libvirt-acl/src/util/virtaudit.c =================================================================== --- libvirt-acl.orig/src/util/virtaudit.c +++ libvirt-acl/src/util/virtaudit.c @@ -30,6 +30,7 @@ #include "virterror_internal.h" #include "logging.h" #include "virtaudit.h" +#include "files.h"
/* Provide the macros in case the header file is old. FIXME: should be removed. */ @@ -133,6 +134,6 @@ void virAuditSend(const char *file ATTRI void virAuditClose(void) { #if HAVE_AUDIT - close(auditfd); + VIR_CLOSE(auditfd);
Must be VIR_FORCE_CLOSE; I'm guessing you didn't have audit turned on in your compile.
Index: libvirt-acl/src/xen/proxy_internal.c @@ -236,12 +237,11 @@ virProxyCloseSocket(xenUnifiedPrivatePtr if (priv->proxy< 0) return(-1);
- ret = close(priv->proxy); + ret = VIR_CLOSE(priv->proxy); if (ret != 0) VIR_WARN("Failed to close socket %d", priv->proxy); else VIR_DEBUG("Closed socket %d", priv->proxy);
Subtle unintended semantic change; you're now printing -1 instead of the fd you just closed; you'll need a temporary.
Index: libvirt-acl/src/xen/xend_internal.c @@ -137,9 +137,7 @@ do_connect(virConnectPtr xend)
if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) == -1) { - serrno = errno; - close(s); - errno = serrno; + VIR_FORCE_CLOSE(s); s = -1;
Redundant line. Overall, I'm impressed with how many lines this is shaving off the code base! I think we settled on a pretty good calling convention. And with that, I've completed my review of v1. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
libvir-list
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
libvir-list-bounces@redhat.com wrote on 10/22/2010 06:27:30 PM: 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.
Can you isolate any of those double-close errors into separate patches which we can apply now, rather than drowning them in the giant patch?
Signed-off-by: Stefan Berger<stefanb@us.ibm.com>
src/phyp/phyp_driver.c | 13 ++--
Resuming...
[...]
Index: libvirt-acl/src/qemu/qemu_monitor.c
@@ -694,8 +695,7 @@ void qemuMonitorClose(qemuMonitorPtr mon if (!mon->closed) { if (mon->watch) virEventRemoveHandle(mon->watch); - if (mon->fd != -1) - close(mon->fd); + VIR_FORCE_CLOSE(mon->fd); /* NB: ordinarily one might immediately set mon->watch to -1 * and mon->fd to -1, but there may be a callback active * that is still relying on these fields being valid. So
Ouch - given that comment, could we be frying a callback by setting mon->fd to -1 in VIR_FORCE_CLOSE? We need to double check this, and possibly use a temporary variable if the callback indeed needs a non-negative mon->fd for a bit longer, or tighten the specification and all existing callbacks to tolerate mon->fd changing to -1.
Probably worth splitting this particular hunk into its own commit, rather than part of the giant patch.
Yes, good idea. Obviously I did not read the comment.
Index: libvirt-acl/src/remote/remote_driver.c =================================================================== --- libvirt-acl.orig/src/remote/remote_driver.c +++ libvirt-acl/src/remote/remote_driver.c @@ -82,6 +82,7 @@ #include "util.h" #include "event.h" #include "ignore-value.h" +#include "files.h"
#define VIR_FROM_THIS VIR_FROM_REMOTE
@@ -711,7 +712,7 @@ doRemoteOpen (virConnectPtr conn, if (errno == ECONNREFUSED&& flags& VIR_DRV_OPEN_REMOTE_AUTOSTART&& trials< 20) { - close(priv->sock); + VIR_FORCE_CLOSE(priv->sock); priv->sock = -1;
This line is now redundant.
Done.
Index: libvirt-acl/src/uml/uml_driver.c @@ -811,7 +811,7 @@ static int umlStartVMDaemon(virConnectPt virDomainObjPtr vm) { const char **argv = NULL, **tmp; const char **progenv = NULL; - int i, ret; + int i, ret, tmpfd; pid_t pid; char *logfile; int logfd = -1;
for (i = 0; i< FD_SETSIZE; i++) - if (FD_ISSET(i,&keepfd)) - close(i); + if (FD_ISSET(i,&keepfd)) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd);
Another awfully large scope for a needed temporary.
Ok.
Index: libvirt-acl/src/util/bridge.c @@ -107,7 +108,7 @@ brShutdown(brControl *ctl) if (!ctl) return;
- close(ctl->fd); + VIR_FORCE_CLOSE(ctl->fd); ctl->fd = 0;
Huh - is this an existing logic bug? Can we end up accidentally double-closing stdin?
I'ld leave the patch for now as it is, i.e., do the VIR_FORCE_CLOSE and remember to investigate. So far, the changed does not have any further negative impact that already isn't there - but it also doesn't solve a potential problem.
Index: libvirt-acl/src/util/logging.c =================================================================== --- libvirt-acl.orig/src/util/logging.c +++ libvirt-acl/src/util/logging.c @@ -40,6 +40,7 @@ #include "util.h" #include "buf.h" #include "threads.h" +#include "files.h"
/* * Macro used to format the message as a string in virLogMessage @@ -603,8 +604,7 @@ static int virLogOutputToFd(const char * static void virLogCloseFd(void *data) { int fd = (long) data;
- if (fd>= 0) - close(fd); + VIR_FORCE_CLOSE(fd);
Should we fix this function to return an int value, and return VIR_CLOSE(fd) so that callers can choose to detect log close failures?
Also that I would delay until further 'cause this may have consequences for those calling the function.
Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -52,6 +52,7 @@ # include "conf/domain_conf.h" # include "virterror_internal.h" # include "uuid.h" +# include "files.h"
# define VIR_FROM_THIS VIR_FROM_NET
@@ -94,7 +95,7 @@ static int nlOpen(void)
static void nlClose(int fd) { - close(fd); + VIR_FORCE_CLOSE(fd);
Likewise?
This here is a close of a netlink socket, which was used to communicate with the kernel. I would just close it and discard the returned value.
Index: libvirt-acl/src/util/util.c =================================================================== --- libvirt-acl.orig/src/util/util.c +++ libvirt-acl/src/util/util.c @@ -71,6 +71,7 @@ #include "memory.h" #include "threads.h" #include "verify.h" +#include "files.h"
#ifndef NSIG # define NSIG 32 @@ -461,6 +462,7 @@ __virExec(const char *const*argv, int pipeerr[2] = {-1,-1}; int childout = -1; int childerr = -1; + int tmpfd;
@@ -568,8 +570,10 @@ __virExec(const char *const*argv, i != childout&& i != childerr&& (!keepfd || - !FD_ISSET(i, keepfd))) - close(i); + !FD_ISSET(i, keepfd))) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd);
I thought this was another large scope for a temporary....
+ }
if (dup2(infd>= 0 ? infd : null, STDIN_FILENO)< 0) { virReportSystemError(errno, @@ -589,14 +593,15 @@ __virExec(const char *const*argv, goto fork_error; }
- if (infd> 0) - close(infd); - close(null); - if (childout> 0) - close(childout); + VIR_FORCE_CLOSE(infd); + VIR_FORCE_CLOSE(null); + tmpfd = childout; /* preserve childout value */ + VIR_FORCE_CLOSE(tmpfd);
...but you needed it here too.
Yes, here I need it twice. So not changing it.
if (childerr> 0&& - childerr != childout) - close(childerr); + childerr != childout) { + VIR_FORCE_CLOSE(childerr); + childout = -1; + }
Looks okay after all - certainly not one of the trivial conversions though :)
Index: libvirt-acl/src/util/virtaudit.c =================================================================== --- libvirt-acl.orig/src/util/virtaudit.c +++ libvirt-acl/src/util/virtaudit.c @@ -30,6 +30,7 @@ #include "virterror_internal.h" #include "logging.h" #include "virtaudit.h" +#include "files.h"
/* Provide the macros in case the header file is old. FIXME: should be removed. */ @@ -133,6 +134,6 @@ void virAuditSend(const char *file ATTRI void virAuditClose(void) { #if HAVE_AUDIT - close(auditfd); + VIR_CLOSE(auditfd);
Must be VIR_FORCE_CLOSE; I'm guessing you didn't have audit turned on in
your compile.
Right...
Index: libvirt-acl/src/xen/proxy_internal.c @@ -236,12 +237,11 @@ virProxyCloseSocket(xenUnifiedPrivatePtr if (priv->proxy< 0) return(-1);
- ret = close(priv->proxy); + ret = VIR_CLOSE(priv->proxy); if (ret != 0) VIR_WARN("Failed to close socket %d", priv->proxy); else VIR_DEBUG("Closed socket %d", priv->proxy);
Subtle unintended semantic change; you're now printing -1 instead of the
fd you just closed; you'll need a temporary.
There was another one like this that I got right :)
Index: libvirt-acl/src/xen/xend_internal.c @@ -137,9 +137,7 @@ do_connect(virConnectPtr xend)
if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) ==
-1) {
- serrno = errno; - close(s); - errno = serrno; + VIR_FORCE_CLOSE(s); s = -1;
Redundant line. Overall, I'm impressed with how many lines this is shaving off the code base! I think we settled on a pretty good calling convention.
Fixed. Will post v2 and if useful a diff(v1,v2). Stefan
And with that, I've completed my review of v1.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/22/2010 09:21 PM, Stefan Berger wrote:
libvir-list-bounces@redhat.com (Eric Blake) wrote on 10/22/2010 06:27:30 PM:
[...]
Index: libvirt-acl/src/qemu/qemu_monitor.c
@@ -694,8 +695,7 @@ void qemuMonitorClose(qemuMonitorPtr mon if (!mon->closed) { if (mon->watch) virEventRemoveHandle(mon->watch); - if (mon->fd != -1) - close(mon->fd); + VIR_FORCE_CLOSE(mon->fd); /* NB: ordinarily one might immediately set mon->watch to -1 * and mon->fd to -1, but there may be a callback active * that is still relying on these fields being valid. So
Ouch - given that comment, could we be frying a callback by setting mon->fd to -1 in VIR_FORCE_CLOSE? We need to double check this, and possibly use a temporary variable if the callback indeed needs a non-negative mon->fd for a bit longer, or tighten the specification and all existing callbacks to tolerate mon->fd changing to -1.
The only possible thing I could think a callback might legitimately use a non-negative fd for would be to answer the question "was this file previously successfully opened?" I would say that if it's being used for that, it would be cleaner to have a separate flag set in the qemuMonitor object that was set when the fd was opened, and never reset. Any other use of the value of fd after it's closed that I can think of is a bug, and changing fd to -1 here would hopefully reveal that bug so it could be fixed.
Index: libvirt-acl/src/util/bridge.c @@ -107,7 +108,7 @@ brShutdown(brControl *ctl) if (!ctl) return;
- close(ctl->fd); + VIR_FORCE_CLOSE(ctl->fd); ctl->fd = 0;
Huh - is this an existing logic bug? Can we end up accidentally double-closing stdin?
Am I missing something? Sure, ctl->fd is set to 0, but then the memory at ctl is immediately freed (in the next line, which doesn't show up in the patch diff), and never referenced again. I'm not even sure why they bothered to set ctl->fd to 0, since it's guaranteed to never be used again.
I'ld leave the patch for now as it is, i.e., do the VIR_FORCE_CLOSE and remember to investigate.
I think we can/should remove both the close() and the ctl->fd = 0, and replace them with VIR_FORCE_CLOSE().
So far, the changed does not have any further negative impact that already isn't there - but it also doesn't solve a potential problem.
Index: libvirt-acl/src/util/logging.c =================================================================== --- libvirt-acl.orig/src/util/logging.c +++ libvirt-acl/src/util/logging.c @@ -40,6 +40,7 @@ #include "util.h" #include "buf.h" #include "threads.h" +#include "files.h"
/* * Macro used to format the message as a string in virLogMessage @@ -603,8 +604,7 @@ static int virLogOutputToFd(const char * static void virLogCloseFd(void *data) { int fd = (long) data;
- if (fd>= 0) - close(fd); + VIR_FORCE_CLOSE(fd);
Should we fix this function to return an int value, and return VIR_CLOSE(fd) so that callers can choose to detect log close failures?
Also that I would delay until further 'cause this may have consequences for those calling the function.
Agreed. That's a good idea, but should be a separate patch.
Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -52,6 +52,7 @@ # include "conf/domain_conf.h" # include "virterror_internal.h" # include "uuid.h" +# include "files.h"
# define VIR_FROM_THIS VIR_FROM_NET
@@ -94,7 +95,7 @@ static int nlOpen(void)
static void nlClose(int fd) { - close(fd); + VIR_FORCE_CLOSE(fd);
Likewise?
This here is a close of a netlink socket, which was used to communicate with the kernel. I would just close it and discard the returned value.
I kind of agree with that, but since a close failure should never happen, when it does happen it may very well indicate something "Very Bad" (eg, corrupted memory, leading to eventual exhaustion of nl sockets (which turns out doesn't take very long)), so we might want to log an error just so they'll know. However, the error could just as well be logged right here, as to return the status to the caller. (And again, I think it can be a separate patch).
participants (4)
-
Eric Blake -
Laine Stump -
Stefan Berger -
Stefan Berger