From c799d150d5e9daec8f970f1be94966683963fda2 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Wed, 18 Mar 2020 12:59:08 +0100 Subject: [PATCH] virprocess: Passthru error from virProcessRunInForkHelper When running a function in a forked child, so far the only thing we could report is exit status of the child and the error message. However, it may be beneficial to the caller to know the actual error that happened in the child. Signed-off-by: Michal Privoznik Reviewed-by: Pavel Mores --- build-aux/syntax-check.mk | 2 +- src/util/virprocess.c | 66 +++++++++++++++++++++++++++++++++++---- tests/commandtest.c | 43 +++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 7 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 3020921be8..2a38c03ba9 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1987,7 +1987,7 @@ exclude_file_name_regexp--sc_flags_usage = \ exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ ^(src/rpc/gendispatch\.pl$$|tests/) -exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$) +exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$|tests/commandtest.c$$) exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \ ^(build-aux/syntax-check\.mk|include/libvirt/virterror\.h|src/remote/remote_daemon_dispatch\.c|src/util/virerror\.c|docs/internals/oomtesting\.html\.in)$$ diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 24135070b7..b126c3c9af 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1126,6 +1126,23 @@ virProcessRunInMountNamespace(pid_t pid G_GNUC_UNUSED, #ifndef WIN32 +typedef struct { + int code; + int domain; + char message[VIR_ERROR_MAX_LENGTH]; + virErrorLevel level; + char str1[VIR_ERROR_MAX_LENGTH]; + char str2[VIR_ERROR_MAX_LENGTH]; + char str3[VIR_ERROR_MAX_LENGTH]; + int int1; + int int2; +} errorData; + +typedef union { + errorData data; + char bindata[sizeof(errorData)]; +} errorDataBin; + static int virProcessRunInForkHelper(int errfd, pid_t ppid, @@ -1134,9 +1151,24 @@ virProcessRunInForkHelper(int errfd, { if (cb(ppid, opaque) < 0) { virErrorPtr err = virGetLastError(); + if (err) { - size_t len = strlen(err->message) + 1; - ignore_value(safewrite(errfd, err->message, len)); + g_autofree errorDataBin *bin = g_new0(errorDataBin, 1); + + bin->data.code = err->code; + bin->data.domain = err->domain; + ignore_value(virStrcpy(bin->data.message, err->message, sizeof(bin->data.message))); + bin->data.level = err->level; + if (err->str1) + ignore_value(virStrcpy(bin->data.str1, err->str1, sizeof(bin->data.str1))); + if (err->str2) + ignore_value(virStrcpy(bin->data.str2, err->str2, sizeof(bin->data.str2))); + if (err->str3) + ignore_value(virStrcpy(bin->data.str3, err->str3, sizeof(bin->data.str3))); + bin->data.int1 = err->int1; + bin->data.int2 = err->int2; + + ignore_value(safewrite(errfd, bin->bindata, sizeof(*bin))); } return -1; @@ -1188,16 +1220,38 @@ virProcessRunInFork(virProcessForkCallback cb, } else { int status; g_autofree char *buf = NULL; + g_autofree errorDataBin *bin = NULL; + int nread; VIR_FORCE_CLOSE(errfd[1]); - ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf)); + nread = virFileReadHeaderFD(errfd[0], sizeof(*bin), &buf); ret = virProcessWait(child, &status, false); if (ret == 0) { ret = status == EXIT_CANCELED ? -1 : status; if (ret) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("child reported (status=%d): %s"), - status, NULLSTR(buf)); + if (nread == sizeof(*bin)) { + bin = g_new0(errorDataBin, 1); + memcpy(bin->bindata, buf, sizeof(*bin)); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("child reported (status=%d): %s"), + status, NULLSTR(bin->data.message)); + + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, + bin->data.domain, + bin->data.code, + bin->data.level, + bin->data.str1, + bin->data.str2, + bin->data.str3, + bin->data.int1, + bin->data.int2, + "%s", bin->data.message); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("child didn't write error (status=%d)"), + status); + } } } } diff --git a/tests/commandtest.c b/tests/commandtest.c index a64aa9ad33..f4a2c67c05 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1257,6 +1257,48 @@ static int test27(const void *unused G_GNUC_UNUSED) } +static int +test28Callback(pid_t pid G_GNUC_UNUSED, + void *opaque G_GNUC_UNUSED) +{ + virReportSystemError(ENODATA, "%s", "some error message"); + return -1; +} + + +static int +test28(const void *unused G_GNUC_UNUSED) +{ + /* Not strictly a virCommand test, but this is the easiest place + * to test this lower-level interface. */ + virErrorPtr err; + + if (virProcessRunInFork(test28Callback, NULL) != -1) { + fprintf(stderr, "virProcessRunInFork did not fail\n"); + return -1; + } + + if (!(err = virGetLastError())) { + fprintf(stderr, "Expected error but got nothing\n"); + return -1; + } + + if (!(err->code == VIR_ERR_SYSTEM_ERROR && + err->domain == 0 && + STREQ(err->message, "some error message: No data available") && + err->level == VIR_ERR_ERROR && + STREQ(err->str1, "%s") && + STREQ(err->str2, "some error message: No data available") && + err->int1 == ENODATA && + err->int2 == -1)) { + fprintf(stderr, "Unexpected error object\n"); + return -1; + } + + return 0; +} + + static int mymain(void) { @@ -1354,6 +1396,7 @@ mymain(void) DO_TEST(test25); DO_TEST(test26); DO_TEST(test27); + DO_TEST(test28); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }