On Wed, Mar 18, 2020 at 06:32:15PM +0100, Michal Privoznik wrote:
> 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 <mprivozn@xxxxxxxxxx>
> ---
> build-aux/syntax-check.mk | 2 +-
> src/util/virprocess.c | 51 ++++++++++++++++++++++++++++++++++++---
> tests/commandtest.c | 43 +++++++++++++++++++++++++++++++++
> 3 files changed, 91 insertions(+), 5 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..e8674402f9 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];
> + /* str3 doesn't seem to be used. Ignore it. */
> + 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,19 @@ virProcessRunInForkHelper(int errfd,
> {
> if (cb(ppid, opaque) < 0) {
> virErrorPtr err = virGetLastError();
> + errorDataBin bin = { 0 };
> +
> if (err) {
> - size_t len = strlen(err->message) + 1;
> - ignore_value(safewrite(errfd, err->message, len));
> + 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;
> + ignore_value(virStrcpy(bin.data.str1, err->str1, sizeof(bin.data.str1)));
> + ignore_value(virStrcpy(bin.data.str2, err->str2, sizeof(bin.data.str2)));
> + bin.data.int1 = err->int1;
> + bin.data.int2 = err->int2;
> +
> + ignore_value(safewrite(errfd, bin.bindata, sizeof(bin)));
> }
>
> return -1;
> @@ -1188,16 +1215,32 @@ virProcessRunInFork(virProcessForkCallback cb,
> } else {
> int status;
> g_autofree char *buf = NULL;
> + errorDataBin bin;
> + 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));
> + status, NULLSTR(bin.data.message));
> +
> + if (nread == sizeof(bin)) {
> + memcpy(bin.bindata, buf, sizeof(bin));
> + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__,
> + bin.data.domain,
> + bin.data.code,
> + bin.data.level,
> + bin.data.str1,
> + bin.data.str2,
> + NULL,
> + bin.data.int1,
> + bin.data.int2,
> + "%s", bin.data.message);
> + }
> }
> }
> }
> 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;
> }
> --
> 2.24.1
>
Reviewed-by: Pavel Mores <pmores@xxxxxxxxxx>
