Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Wednesday in 2020, 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 };
> +

Consider allocating this on the heap instead of a 3K+ stack variable.

Jano

>         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;

Same here.

Jano

Attachment: signature.asc
Description: PGP signature