2010/10/15 Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>:
> Â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@xxxxxxxxxx>
>
> ---
> Â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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list