Re: [libvirt] [PATCH] introduce VIR_CLOSE to be used rather than close()

 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(a)us.ibm.com&gt; ---  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;  }