"Daniel P. Berrange" <berrange@redhat.com> wrote:
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.
Looks fine. ACK one "would be nice" comment:
diff -r 4f44b07c47c1 src/util.c ... 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"),
Since this code is going to be used in so many contexts, now, it'd be nice to report which signal (if any) and the precise exit status value.
+ argv[0]); + return -1;