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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux