Cole Robinson wrote:
> The attached patch adds some extra logging to the
> virRun command. The full argv is now logged with
> DEBUG, as well as the commands stdout and stderr
> (if anything is found).
>
> Also, if the command returns an error, we raise
> the stderr content with the reported error msg.
> This will significantly help with debugging certain
> issues, particularly with the storage driver which
> makes heavy use of virRun.
I think the idea is great. I'm a little worried about the implementation, though.
> --- a/src/util.c
> +++ b/src/util.c
> @@ -54,6 +54,9 @@
> #include "memory.h"
> #include "util-lib.c"
>
> +#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
> +#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
Leftover cruft, it looks like. It's not used anywhere, and you can just use
DEBUG() for the same thing.
> +
> #ifndef NSIG
> # define NSIG 32
> #endif
> @@ -404,10 +407,19 @@ int
> virRun(virConnectPtr conn,
> const char *const*argv,
> int *status) {
> - int childpid, exitstatus, ret;
> + int childpid, exitstatus, ret, i;
> + int errfd = -1, outfd = -1;
> + char out[256] = "\0", err[256] = "\0", cmd[512] = "\0";;
> +
> + /* Log argv */
> + for (i = 0; argv[i] != NULL; ++i) {
> + strncat(cmd, " ", sizeof(cmd) - strlen(cmd) - 1);
> + strncat(cmd, argv[i], sizeof(cmd) - strlen(cmd) - 1);
> + }
What happens if you have enough arguments that you overrun 512 bytes in the cmd
array? I guess the sizeof(cmd) - strlen(cmd) - 1 will protect you from running
off the end of the buffer, but you'll truncate the output. There are some
examples elsewhere in the code (src/qemu_conf.c, I think) that properly build up
the whole thing; it's probably worthwhile to look there.
> + DEBUG("Running command '%s'", cmd);
>
> if ((ret = virExec(conn, argv, NULL, NULL,
> - &childpid, -1, NULL, NULL, VIR_EXEC_NONE)) < 0)
> + &childpid, -1, &outfd, &errfd, VIR_EXEC_NONE)) < 0)
> return ret;
>
> while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR);
> @@ -418,16 +430,31 @@ virRun(virConnectPtr conn,
> return -1;
> }
>
> + /* Log command output */
> + if (outfd) {
> + ret = saferead(outfd, out, sizeof(out)-1);
> + err[ret < 0 ? 0 : ret] = '\0';
> + if (*out)
> + DEBUG("Command stdout: %s", out);
Don't you also want to close(outfd) here?
> + }
> + if (errfd) {
> + ret = saferead(errfd, err, sizeof(err)-1);
> + err[ret < 0 ? 0 : ret] = '\0';
> + if (*err)
> + DEBUG("Command stderr: %s", err);
Ditto for errfd.
Chris Lalancette
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list