Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper
On 24. 3. 2020 11:52, Pino Toscano wrote: > On Tuesday, 24 March 2020 11:48:17 CET Daniel P. Berrangé wrote: >> IIUC the problem here is that the STREQ check is assuming that the >> ENODATA errno results in the string "No data available". The strings >> are not standardized by POSIX AFAIK, so C libraries can use any reasonable >> text for them. So this check is not portable. >> >> Perhaps its enough to use STRPREFIX(err->str2, "some error message:") ? > > Or maybe use g_strerror to get the error message of ENODATA, and > compare it to the actual message got in the test. > Ah indeed, that is the problem. I've went with Pino's suggestion and proposed a patch for it. Michal
Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper
On Tuesday, 24 March 2020 11:48:17 CET Daniel P. Berrangé wrote:
> 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
> > ---
> > 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/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;
> > +}
>
> This new test fails on FreeBSD
>
> $ VIR_TEST_DEBUG=1 VIR_TEST_RANGE=28 ./commandtest
> TEST: commandtest
> 28) Command Exec test28 test ...
> Unexpected error object
> libvirt: error : some error message: Input/output error
> FAILED
>
>
> IIUC the problem here is that the STREQ check is assuming that the
> ENODATA errno results in the string "No data available". The strings
> are not standardized by POSIX AFAIK, so C libraries can use any reasonable
> text for them. So this check is not portable.
>
> Perhaps its enough to use STRPREFIX(err->str2, "some error message:") ?
Or maybe use g_strerror to get the error message of ENODATA, and
compare it to the actual message got in the test.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper
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
> ---
> 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/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;
> +}
This new test fails on FreeBSD
$ VIR_TEST_DEBUG=1 VIR_TEST_RANGE=28 ./commandtest
TEST: commandtest
28) Command Exec test28 test ...
Unexpected error object
libvirt: error : some error message: Input/output error
FAILED
IIUC the problem here is that the STREQ check is assuming that the
ENODATA errno results in the string "No data available". The strings
are not standardized by POSIX AFAIK, so C libraries can use any reasonable
text for them. So this check is not portable.
Perhaps its enough to use STRPREFIX(err->str2, "some error message:") ?
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper
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
---
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
signature.asc
Description: PGP signature
Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper
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
> ---
> 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, ));
> +nread = virFileReadHeaderFD(errfd[0], sizeof(bin), );
> ret = virProcessWait(child, , 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)
> +{
> +
[PATCH 5/6] 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
---
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, ));
+nread = virFileReadHeaderFD(errfd[0], sizeof(bin), );
ret = virProcessWait(child, , 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) {
