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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list