2010/10/15 Stefan Berger <stefanb@linux.vnet.ibm.com>:
Since bugs due to double-closed filedescriptors are difficult to track down in a multi-threaded system, I am introducing the VIR_CLOSE(fd) macro to help avoid mistakes here.
There are lots of places where close() is being used. In this patch I am only cleaning up usage of close() in src/conf where the problems were.
I also dare to declare close() as being deprecated in libvirt code base (HACKING). :-)
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- HACKING | 11 +++++++++++ src/Makefile.am | 3 ++- src/conf/domain_conf.c | 14 ++++++++------ src/conf/network_conf.c | 9 +++++---- src/conf/nwfilter_conf.c | 17 +++++++++-------- src/conf/storage_conf.c | 9 +++++---- src/conf/storage_encryption_conf.c | 5 +++-- src/util/files.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/files.h | 37 +++++++++++++++++++++++++++++++++++++ 9 files changed, 117 insertions(+), 25 deletions(-)
Index: libvirt-acl/src/util/files.c =================================================================== --- /dev/null +++ libvirt-acl/src/util/files.c @@ -0,0 +1,37 @@ +/* + * memory.c: safer file handling + * + * Copyright (C) 2010 IBM Corporation + * Copyright (C) 2010 Stefan Berger + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <unistd.h> + +#include "files.h" + +int virClose(int *fdptr) +{ + int rc; + + if (*fdptr >= 0) { + rc = close(*fdptr); + *fdptr = -1; + } else + rc = 0; + return rc; +} Index: libvirt-acl/src/util/files.h =================================================================== --- /dev/null +++ libvirt-acl/src/util/files.h @@ -0,0 +1,37 @@ +/* + * files.h: safer file handling + * + * Copyright (C) 2010 IBM Corporation + * Copyright (C) 2010 Stefan Berger + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + + +#ifndef __VIR_FILES_H_ +# define __VIR_FILES_H_ + +# include "internal.h" + +/* Don't call these directly - use the macros below */ +int virClose(int *fdptr); + +# define VIR_CLOSE(FD) \ + virClose(&(FD)) + + +#endif /* __VIR_FILES_H */ +
Index: libvirt-acl/src/conf/domain_conf.c =================================================================== --- libvirt-acl.orig/src/conf/domain_conf.c +++ libvirt-acl/src/conf/domain_conf.c @@ -46,6 +46,7 @@ #include "nwfilter_conf.h" #include "ignore-value.h" #include "storage_file.h" +#include "files.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -6798,17 +6799,18 @@ int virDomainSaveXML(const char *configD goto cleanup; }
- if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot save config file '%s'"), configFile); - goto cleanup; + goto cleanup_free; }
ret = 0; cleanup: - if (fd != -1) - close(fd); + VIR_CLOSE(fd); + + cleanup_free: VIR_FREE(configFile); return ret; }
Why did you add a new label here? You build VIR_CLOSE in a way that allows safe double invocation. Therefore, this is just fine, isn't it: - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot save config file '%s'"), configFile); goto cleanup; } ret = 0; cleanup: - if (fd != -1) - close(fd); + VIR_CLOSE(fd); VIR_FREE(configFile); return ret; } This pattern reoccurs several times. Matthias