Re: [libvirt] PATCH: 1/5: Improved error reporting

There are several system calls in the virExec function for which we don't or can't report errors. This patch does a couple of things to improve the situation. It moves the code for setting non-block/close-exec to before the fork() so we can report errors for it. It removes the 'dom' and 'net' params from the ReportError function since we deprecated those long ago and all callers simply pass in NULL. It resets the 'virErrorHandler' callback to NULL in the child, so that errors raised will get reported to stderr instead of invoking a callback which is likely no longer valid in the child process. It reports failures to exec the binary and dup file descriptors. All errors in the child will end up on stderr, so they will at least be visible on the parent's console, or a logfile if one was setup for the child. Previously there would just be silent failure. Daniel diff -r 4f44b07c47c1 src/util.c --- a/src/util.c Mon Aug 11 20:45:28 2008 +0100 +++ b/src/util.c Tue Aug 12 14:52:41 2008 +0100 @@ -61,9 +61,7 @@ #ifndef PROXY static void ReportError(virConnectPtr conn, - virDomainPtr dom, - virNetworkPtr net, - int code, const char *fmt, ...) { + int code, const char *fmt, ...) { va_list args; char errorMessage[MAX_ERROR_LEN]; @@ -74,7 +72,7 @@ } else { errorMessage[0] = '\0'; } - __virRaiseError(conn, dom, net, VIR_FROM_NONE, code, VIR_ERR_ERROR, + __virRaiseError(conn, NULL, NULL, VIR_FROM_NONE, code, VIR_ERR_ERROR, NULL, NULL, NULL, -1, -1, "%s", errorMessage); } @@ -109,7 +107,7 @@ int pipeerr[2] = {-1,-1}; if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) { - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot open %s: %s"), _PATH_DEVNULL, strerror(errno)); goto cleanup; @@ -117,13 +115,41 @@ if ((outfd != NULL && pipe(pipeout) < 0) || (errfd != NULL && pipe(pipeerr) < 0)) { - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot create pipe: %s"), strerror(errno)); goto cleanup; } + if (outfd) { + if(non_block && + virSetNonBlock(pipeout[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + + if(virSetCloseExec(pipeout[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + } + if (errfd) { + if(non_block && + virSetNonBlock(pipeerr[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + if(virSetCloseExec(pipeerr[0]) == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + } + if ((pid = fork()) < 0) { - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot fork child process: %s"), strerror(errno)); goto cleanup; } @@ -132,26 +158,10 @@ close(null); if (outfd) { close(pipeout[1]); - if(non_block) - if(virSetNonBlock(pipeout[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set non-blocking file descriptor flag")); - - if(virSetCloseExec(pipeout[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set close-on-exec file descriptor flag")); *outfd = pipeout[0]; } if (errfd) { close(pipeerr[1]); - if(non_block) - if(virSetNonBlock(pipeerr[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set non-blocking file descriptor flag")); - - if(virSetCloseExec(pipeerr[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set close-on-exec file descriptor flag")); *errfd = pipeerr[0]; } *retpid = pid; @@ -160,23 +170,47 @@ /* child */ - if (pipeout[0] > 0 && close(pipeout[0]) < 0) + /* Don't want to report errors against this accidentally, so + just discard it */ + conn = NULL; + /* Remove any error callback too, so errors in child now + get sent to stderr where they stand a fighting chance + of being seen / logged */ + virSetErrorFunc(NULL, NULL); + + if (pipeout[0] > 0) + close(pipeout[0]); + if (pipeerr[0] > 0) + close(pipeerr[0]); + + + if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stdin file handle: %s"), strerror(errno)); _exit(1); - if (pipeerr[0] > 0 && close(pipeerr[0]) < 0) + } +#ifndef ENABLE_DEBUG + if (dup2(pipeout[1] > 0 ? pipeout[1] : null, STDOUT_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stdout file handle: %s"), strerror(errno)); _exit(1); - - if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) + } + if (dup2(pipeerr[1] > 0 ? pipeerr[1] : null, STDERR_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stderr file handle: %s"), strerror(errno)); _exit(1); -#ifndef ENABLE_DEBUG - if (dup2(pipeout[1] > 0 ? pipeout[1] : null, STDOUT_FILENO) < 0) + } +#else /* ENABLE_DEBUG */ + if (pipeout[1] > 0 && dup2(pipeout[1], STDOUT_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stderr file handle: %s"), strerror(errno)); _exit(1); - if (dup2(pipeerr[1] > 0 ? pipeerr[1] : null, STDERR_FILENO) < 0) + } + if (pipeerr[1] > 0 && dup2(pipeerr[1], STDERR_FILENO) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("failed to setup stdout file handle: %s"), strerror(errno)); _exit(1); -#else /* ENABLE_DEBUG */ - if (pipeout[1] > 0 && dup2(pipeout[1], STDOUT_FILENO) < 0) - _exit(1); - if (pipeerr[1] > 0 && dup2(pipeerr[1], STDERR_FILENO) < 0) - _exit(1); + } #endif /* ENABLE_DEBUG */ close(null); @@ -187,11 +221,21 @@ execvp(argv[0], (char **) argv); + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot execute binary '%s': %s"), + argv[0], strerror(errno)); + _exit(1); return 0; cleanup: + /* This is cleanup of parent process only - child + should never jump here on error */ + + /* NB we don't ReportError() on any failures here + because the code which jumped hre already raised + an error condition which we must not overwrite */ if (pipeerr[0] > 0) close(pipeerr[0]); if (pipeerr[1] > 0) @@ -246,12 +290,22 @@ return ret; while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR); - if (ret == -1) + if (ret == -1) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot wait for '%s': %s"), + argv[0], strerror(errno)); return -1; + } if (status == NULL) { errno = EINVAL; - return (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) ? 0 : -1; + if (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) + return 0; + + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("%s exited with non-zero status %s"), + argv[0]); + return -1; } else { *status = exitstatus; return 0; @@ -268,7 +322,7 @@ int *outfd ATTRIBUTE_UNUSED, int *errfd ATTRIBUTE_UNUSED) { - ReportError (conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); + ReportError (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); return -1; } @@ -280,7 +334,7 @@ int *outfd ATTRIBUTE_UNUSED, int *errfd ATTRIBUTE_UNUSED) { - ReportError (conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); + ReportError (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); return -1; } -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|