[libvirt] [PATCH] bye to close(), welcome to VIR_(FORCE_)CLOSE()
Eric Blake
eblake at redhat.com
Fri Oct 22 22:27:30 UTC 2010
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 at 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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list