[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] bye to close(), welcome to VIR_(FORCE_)CLOSE()



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 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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]