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

[Stefan Berger]

On Mon, 2010-10-25 at 13:44 +0100, Daniel P. Berrange wrote:
> On Mon, Oct 25, 2010 at 08:13:19AM -0400, Stefan Berger wrote:
> > Index: libvirt-acl/src/libvirt.c
> > @@ -1544,14 +1544,11 @@ cleanup:
> >              vethDelete(veths[i]);
> >          VIR_FREE(veths[i]);
> >      }
> > -    if (rc != 0 && priv->monitor != -1) {
> > -        close(priv->monitor);
> > -        priv->monitor = -1;
> > -    }
> > -    if (parentTty != -1)
> > -        close(parentTty);
> > -    if (logfd != -1)
> > -        close(logfd);
> > +    if (rc != 0)
> > +        VIR_FORCE_CLOSE(priv->monitor);
> > +    VIR_FORCE_CLOSE(parentTty);
> > +    if (VIR_CLOSE(logfd) < 0)
> > +        virReportSystemError(errno, "%s", _("could not close logfile"));
> 
> This is reporting an error without returning an error code, so the
> caller will still see success. 
> 
This hunk is in lxc_driver.c in the function lxcVmStart(). Now if in the
cleanup the logfile cannot be closed, that doesn't mean that the VM
could not be started. I am not sure how to handle this correctly, but if
we report an error, we'd probably need to terminate the VM as well... so
is a VIR_FORCE_CLOSE() the proper solution here?


> > @@ -2011,8 +2008,7 @@ lxcReconnectVM(void *payload, const char
> >  
> > @@ -457,11 +458,15 @@ phypUUIDTable_WriteFile(virConnectPtr co
> >          }
> >      }
> >  
> > -    close(fd);
> > +    if (VIR_CLOSE(fd) < 0)
> > +        virReportSystemError(errno, _("Could not close %s\n"),
> > +                             local_file);
> >      return 0;
> 
> Again, reporting an error while returning success.

Yes, here I can do better and do a 'goto err'.
> 
> >  
> >    err:
> > -    close(fd);
> > +    if (VIR_CLOSE(fd) < 0)
> > +        virReportSystemError(errno, _("Could not close %s\n"),
> > +                             local_file);
> >      return -1;
> >  }
> 
> This is likely blowing away a previously reported error.

Ok, so should I change this to VIR_FORCE_CLOSE()?
> 
> > @@ -764,7 +769,9 @@ phypUUIDTable_Pull(virConnectPtr conn)
> >          }
> >          break;
> >      }
> > -    close(fd);
> > +    if (VIR_CLOSE(fd) < 0)
> > +        virReportSystemError(errno, _("Could not close %s\n"),
> > +                             local_file);
> >      goto exit;
> 
> Reporting error while returning success

Will do a 'goto err' here as well.
> 
> > @@ -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"));
> > +    }
> 
> Reporting error while returning success
> 
> 
.. and change this here to VIR_FORCE_CLOSE.

> > Index: libvirt-acl/src/storage/storage_backend_fs.c
> 
> > @@ -94,7 +95,9 @@ static int nlOpen(void)
> >  
> >  static void nlClose(int fd)
> >  {
> > -    close(fd);
> > +    if (VIR_CLOSE(fd) < 0)
> > +        virReportSystemError(errno,
> > +                             "%s",_("cannot close netlink socket"));
> >  }
> 
> No return status at all - this function likely shouldn't even
> exist. Should be replaced with direct calls to VIR_FORCE_CLOSE
> and VIR_CLOSE as appropriate, returning correct error codes
> if it wants to handle close failures.

I'll remove this function. It existed due to nlOpen() existing.

> > @@ -418,17 +419,13 @@ virHookCall(int driver, const char *id, 
> >      }
> >  
> >  cleanup:
> > -    if (pipefd[0] >= 0) {
> > -        if (close(pipefd[0]) < 0) {
> > -            virReportSystemError(errno, "%s",
> > -                             _("unable to close pipe for hook input"));
> > -        }
> > -    }
> > -    if (pipefd[1] >= 0) {
> > -        if (close(pipefd[1]) < 0) {
> > -            virReportSystemError(errno, "%s",
> > -                             _("unable to close pipe for hook input"));
> > -        }
> > +    if (VIR_CLOSE(pipefd[0]) < 0) {
> > +        virReportSystemError(errno, "%s",
> > +                         _("unable to close pipe for hook input"));
> > +    }
> > +    if (VIR_CLOSE(pipefd[1]) < 0) {
> > +        virReportSystemError(errno, "%s",
> > +                         _("unable to close pipe for hook input"));
> >      }
> 
> Reporting errors while returning success.

Use VIR_FORCE_CLOSE() here and convert to not report an error?


Stefan

> 
> Regards,
> Daniel