[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@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/Makefile.am =================================================================== --- libvirt-acl.orig/src/Makefile.am +++ libvirt-acl/src/Makefile.am @@ -82,7 +82,8 @@ UTIL_SOURCES = \ util/uuid.c util/uuid.h \ util/util.c util/util.h \ util/xml.c util/xml.h \ - util/virterror.c util/virterror_internal.h + util/virterror.c util/virterror_internal.h \ + util/files.c util/files.h EXTRA_DIST += util/threads-pthread.c util/threads-win32.c 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; } @@ -7765,10 +7767,10 @@ int virDomainDiskDefForeachPath(virDomai } if (virStorageFileGetMetadataFromFD(path, fd, format, &meta) < 0) { - close(fd); + VIR_CLOSE(fd); goto cleanup; } - close(fd); + VIR_CLOSE(fd); if (virHashAddEntry(paths, path, (void*)0x1) < 0) { virReportOOMError(); Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -45,6 +45,7 @@ #include "nwfilter_conf.h" #include "domain_conf.h" #include "c-ctype.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -2193,19 +2194,19 @@ int virNWFilterSaveXML(const char *confi 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; @@ -2604,19 +2605,19 @@ virNWFilterPoolObjSaveDef(virNWFilterDri goto cleanup; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot save config file %s"), pool->configFile); - goto cleanup; + goto cleanup_free; } ret = 0; cleanup: - if (fd != -1) - close(fd); + VIR_CLOSE(fd); + cleanup_free: VIR_FREE(xml); return ret; Index: libvirt-acl/src/conf/storage_conf.c =================================================================== --- libvirt-acl.orig/src/conf/storage_conf.c +++ libvirt-acl/src/conf/storage_conf.c @@ -43,6 +43,7 @@ #include "buf.h" #include "util.h" #include "memory.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1560,19 +1561,19 @@ virStoragePoolObjSaveDef(virStorageDrive goto cleanup; } - if (close(fd) < 0) { + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot save config file %s"), pool->configFile); - goto cleanup; + goto cleanup_free; } ret = 0; cleanup: - if (fd != -1) - close(fd); + VIR_CLOSE(fd); + cleanup_free: VIR_FREE(xml); return ret; Index: libvirt-acl/src/conf/network_conf.c =================================================================== --- libvirt-acl.orig/src/conf/network_conf.c +++ libvirt-acl/src/conf/network_conf.c @@ -43,6 +43,7 @@ #include "util.h" #include "buf.h" #include "c-ctype.h" +#include "files.h" #define MAX_BRIDGE_ID 256 #define VIR_FROM_THIS VIR_FROM_NETWORK @@ -687,19 +688,19 @@ int virNetworkSaveXML(const char *config 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; Index: libvirt-acl/src/conf/storage_encryption_conf.c =================================================================== --- libvirt-acl.orig/src/conf/storage_encryption_conf.c +++ libvirt-acl/src/conf/storage_encryption_conf.c @@ -35,6 +35,7 @@ #include "xml.h" #include "virterror_internal.h" #include "uuid.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -286,12 +287,12 @@ virStorageGenerateQcowPassphrase(unsigne if (r <= 0) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot read from /dev/urandom")); - close(fd); + VIR_CLOSE(fd); return -1; } if (dest[i] >= 0x20 && dest[i] <= 0x7E) i++; /* Got an acceptable character */ } - close(fd); + VIR_CLOSE(fd); return 0; } Index: libvirt-acl/HACKING =================================================================== --- libvirt-acl.orig/HACKING +++ libvirt-acl/HACKING @@ -318,6 +318,17 @@ routines, use the macros from memory.h VIR_FREE(domain); +File handling +============= + +Use of the close() API is deprecated in libvirt code base to help avoiding +double-closing of a filedescriptor. Instead of this API, use the macro from +files.h + + - eg close a filedescriptor + + VIR_CLOSE(fd); + String comparisons ==================
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
On 10/15/2010 12:54 PM, Matthias Bolte wrote:
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>
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:
I had it without this new label and then changed it since I think it's still good practice to not try to invoke the function twice, even though nothing can go wrong. Stefan
On 10/15/2010 01:01 PM, Stefan Berger wrote:
On 10/15/2010 12:54 PM, Matthias Bolte wrote:
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>
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:
I had it without this new label and then changed it since I think it's still good practice to not try to invoke the function twice, even though nothing can go wrong.
Stefan
VIR_FREE() is commonly used in the way that Matthias describes, so I think it would be consistent to use VIR_CLOSE in this way as well. Invoking it unconditionally simplifies the code, and eliminates possible confusion when someone adds new code and isn't sure which label they should goto on failure (or maybe they think they are sure, but make the wrong assumption, thus leading to a leaked fd as a result of trying to eliminate possible double-closed fd's ;-)
On 10/15/2010 11:12 AM, Laine Stump wrote:
in a multi-threaded system, I am introducing the VIR_CLOSE(fd) macro to help avoid mistakes here.
VIR_FREE() is commonly used in the way that Matthias describes, so I think it would be consistent to use VIR_CLOSE in this way as well.
Invoking it unconditionally simplifies the code, and eliminates possible confusion when someone adds new code and isn't sure which label they should goto on failure (or maybe they think they are sure, but make the wrong assumption, thus leading to a leaked fd as a result of trying to eliminate possible double-closed fd's ;-)
I agree - the whole point of adding the macro is to make it so that we have a smaller maintenance burden - fewer labels is a good thing. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/15/2010 10:15 AM, Stefan Berger wrote:
Index: libvirt-acl/src/util/files.c =================================================================== --- /dev/null +++ libvirt-acl/src/util/files.c
+ */ + +#include <unistd.h>
Oops, need to include <config.h> first.
+++ libvirt-acl/src/util/files.h @@ -0,0 +1,37 @@
+#ifndef __VIR_FILES_H_ +# define __VIR_FILES_H_ + +# include "internal.h" + +/* Don't call these directly - use the macros below */
s/these/this/
+int virClose(int *fdptr);
Needs an ATTRIBUTE_NONNULL(1). I'm also debating whether it needs ATTRIBUTE_RETURN_CHECK; normally, close() fails in very few situations, and in cleanup paths, you tend to already have another error more important so you can ignore the secondary close() failures. So I'm probably 80-20 against adding ATTRIBUTE_RETURN_CHECK.
+ +# define VIR_CLOSE(FD) \ + virClose(&(FD)) + + +#endif /* __VIR_FILES_H */ +
No change in src/libvirt_private.syms?
Index: libvirt-acl/HACKING =================================================================== --- libvirt-acl.orig/HACKING +++ libvirt-acl/HACKING
Yuck - we STILL don't have the auto-generation in place; edits should be to docs/hacking.html.in, and HACKING _should_ be generated from that. Until then, you need to touch both files :(
@@ -318,6 +318,17 @@ routines, use the macros from memory.h VIR_FREE(domain);
+File handling +============= + +Use of the close() API is deprecated in libvirt code base to help avoiding +double-closing of a filedescriptor. Instead of this API, use the macro from +files.h + + - eg close a filedescriptor
s/filedescriptor/file descriptor/g
+ + VIR_CLOSE(fd);
Although I'm 80-20 against warning on unused results, we might as well at least make the example demonstrate that the results are valid: if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("failed to close file")); } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/15/2010 11:32 AM, Eric Blake wrote:
+int virClose(int *fdptr);
Needs an ATTRIBUTE_NONNULL(1). I'm also debating whether it needs ATTRIBUTE_RETURN_CHECK; normally, close() fails in very few situations, and in cleanup paths, you tend to already have another error more important so you can ignore the secondary close() failures. So I'm probably 80-20 against adding ATTRIBUTE_RETURN_CHECK.
+ +# define VIR_CLOSE(FD) \ + virClose(&(FD))
Thinking about it a bit more, what if we provide two variants? int virClose(int *fdptr, bool preserve_errno) { int saved_errno; int rc; if (*fdptr >= 0) { if (preserve_errno) saved_errno = errno; rc = close(*fdptr); *fdptr = -1; if (preserve_errno) errno = saved_errno; } else rc = 0; return rc; } int virClose(int *fdptr) ATTRIBUTE_RETURN_CHECK; /* For use on normal paths; caller must check return value, and failure sets errno per close(). */ #define VIR_CLOSE(FD) virClose(&(FD),false); /* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ #define VIR_FORCE_CLOSE(FD) ignore_value(virClose(&(FD),true)) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/15/2010 01:45 PM, Eric Blake wrote:
On 10/15/2010 11:32 AM, Eric Blake wrote:
+int virClose(int *fdptr);
Needs an ATTRIBUTE_NONNULL(1). I'm also debating whether it needs ATTRIBUTE_RETURN_CHECK; normally, close() fails in very few situations, and in cleanup paths, you tend to already have another error more important so you can ignore the secondary close() failures. So I'm probably 80-20 against adding ATTRIBUTE_RETURN_CHECK.
+ +# define VIR_CLOSE(FD) \ + virClose(&(FD))
Thinking about it a bit more, what if we provide two variants?
int virClose(int *fdptr, bool preserve_errno) { int saved_errno; int rc;
if (*fdptr >= 0) { if (preserve_errno) saved_errno = errno; rc = close(*fdptr); *fdptr = -1; if (preserve_errno) errno = saved_errno; } else rc = 0;
return rc; }
int virClose(int *fdptr) ATTRIBUTE_RETURN_CHECK;
'approximately', yes
/* For use on normal paths; caller must check return value, and failure sets errno per close(). */ #define VIR_CLOSE(FD) virClose(&(FD),false);
/* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ #define VIR_FORCE_CLOSE(FD) ignore_value(virClose(&(FD),true))
Fine by me. I am picking this up for the next patch 'as-is'. You can edit the file header (copyright and so on). Stefan
On 10/15/2010 01:32 PM, Eric Blake wrote:
On 10/15/2010 10:15 AM, Stefan Berger wrote:
Index: libvirt-acl/src/util/files.c =================================================================== --- /dev/null +++ libvirt-acl/src/util/files.c
+ */ + +#include <unistd.h>
Oops, need to include <config.h> first. Ok.
+++ libvirt-acl/src/util/files.h @@ -0,0 +1,37 @@
+#ifndef __VIR_FILES_H_ +# define __VIR_FILES_H_ + +# include "internal.h" + +/* Don't call these directly - use the macros below */
s/these/this/ Ok.
+int virClose(int *fdptr);
Needs an ATTRIBUTE_NONNULL(1). I'm also debating whether it needs ATTRIBUTE_RETURN_CHECK; normally, close() fails in very few situations, and in cleanup paths, you tend to already have another error more important so you can ignore the secondary close() failures. So I'm probably 80-20 against adding ATTRIBUTE_RETURN_CHECK.
Your other mail...
+ +# define VIR_CLOSE(FD) \ + virClose(&(FD)) + + +#endif /* __VIR_FILES_H */ +
No change in src/libvirt_private.syms?
Was waiting for the linker to complain and it did not.
Index: libvirt-acl/HACKING =================================================================== --- libvirt-acl.orig/HACKING +++ libvirt-acl/HACKING
Yuck - we STILL don't have the auto-generation in place; edits should be to docs/hacking.html.in, and HACKING _should_ be generated from that. Until then, you need to touch both files :(
Ok, will do.
@@ -318,6 +318,17 @@ routines, use the macros from memory.h VIR_FREE(domain);
+File handling +============= + +Use of the close() API is deprecated in libvirt code base to help avoiding +double-closing of a filedescriptor. Instead of this API, use the macro from +files.h + + - eg close a filedescriptor
s/filedescriptor/file descriptor/g
ok.
+ + VIR_CLOSE(fd);
Although I'm 80-20 against warning on unused results, we might as well at least make the example demonstrate that the results are valid:
if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("failed to close file")); }
Replacing the example. Stefan
On 10/15/2010 12:26 PM, Stefan Berger wrote:
No change in src/libvirt_private.syms?
Was waiting for the linker to complain and it did not.
My understanding is that the *.syms files are important mainly if you build with --with-driver-modules, but that defaults to no. Without it, you get a single binary (where the link is satisfied) rather than multiple .so (where the .syms file is essential to avoid link failures in the various driver modules), so the default build hides whether you really need to add it. Given that danpb's recent 4-patch series to add auditing points had to export a lot more symbols, we obviously don't do much testing of module builds. So I'd feel better seeing it added, even if you haven't yet configured in a manner that would provoke link failure without it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eric Blake -
Laine Stump -
Matthias Bolte -
Stefan Berger