| Message ID | 20190827120221.15725-3-yury-kotov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | UUID validation during migration | expand |
Hi On Tue, Aug 27, 2019 at 4:09 PM Yury Kotov <yury-kotov@yandex-team.ru> wrote: > > Add qtest_set_expected_status function to set expected exit status of > child process. By default expected exit status is 0. > > Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru> > --- > tests/libqtest.c | 14 +++++++++++--- > tests/libqtest.h | 9 +++++++++ > 2 files changed, 20 insertions(+), 3 deletions(-) > I sent a vary similar patch with v1 of dbus-vmstate, and dropped it because it no longer needs it in v2 (for now) "tests: add qtest_set_exit_status()". Your function is better named already. > diff --git a/tests/libqtest.c b/tests/libqtest.c > index 2713b86cf7..118d779c1b 100644 > --- a/tests/libqtest.c > +++ b/tests/libqtest.c > @@ -43,6 +43,7 @@ struct QTestState > int qmp_fd; > pid_t qemu_pid; /* our child QEMU process */ > int wstatus; > + int expected_status; > bool big_endian; > bool irq_level[MAX_IRQ]; > GString *rx; > @@ -113,6 +114,11 @@ bool qtest_probe_child(QTestState *s) > return false; > } > > +void qtest_set_expected_status(QTestState *s, int status) > +{ > + s->expected_status = status; > +} > + > static void kill_qemu(QTestState *s) > { > pid_t pid = s->qemu_pid; > @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s) > * fishy and should be logged with as much detail as possible. > */ > wstatus = s->wstatus; > - if (wstatus) { > + if (WEXITSTATUS(wstatus) != s->expected_status) { Shouldn't it check WEXITSTATUS value only when WIFEXITED ? > if (WIFEXITED(wstatus)) { > fprintf(stderr, "%s:%d: kill_qemu() tried to terminate QEMU " > - "process but encountered exit status %d\n", > - __FILE__, __LINE__, WEXITSTATUS(wstatus)); > + "process but encountered exit status %d (expected %d)\n", > + __FILE__, __LINE__, WEXITSTATUS(wstatus), > + s->expected_status); > } else if (WIFSIGNALED(wstatus)) { > int sig = WTERMSIG(wstatus); > const char *signame = strsignal(sig) ?: "unknown ???"; > @@ -248,6 +255,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) > g_test_message("starting QEMU: %s", command); > > s->wstatus = 0; > + s->expected_status = 0; > s->qemu_pid = fork(); > if (s->qemu_pid == 0) { > setenv("QEMU_AUDIO_DRV", "none", true); > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 07ea35867c..c00bca94af 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -997,4 +997,13 @@ void qmp_assert_error_class(QDict *rsp, const char *class); > */ > bool qtest_probe_child(QTestState *s); > > +/** > + * qtest_set_expected_status: > + * @s: QTestState instance to operate on. > + * @status: an expected exit status. > + * > + * Set expected exit status of the child. > + */ > +void qtest_set_expected_status(QTestState *s, int status); > + > #endif > -- > 2.22.0 > >
On 8/27/19 7:02 AM, Yury Kotov wrote:
In the subject: 'Allow to $verb' is not idiomatic; either 'Allow
$subject to $verb' or 'Allow ${verb}ing' sound better. In this case,
I'd go with:
tests/libqtest: Allow setting expected exit status
> Add qtest_set_expected_status function to set expected exit status of
> child process. By default expected exit status is 0.
>
> @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s)
> * fishy and should be logged with as much detail as possible.
> */
> wstatus = s->wstatus;
> - if (wstatus) {
> + if (WEXITSTATUS(wstatus) != s->expected_status) {
> if (WIFEXITED(wstatus)) {
Wrong ordering. WEXITSTATUS() is not reliable unless WIFEXITED() is true.
27.08.2019, 16:53, "Marc-André Lureau" <marcandre.lureau@gmail.com>: > Hi > > On Tue, Aug 27, 2019 at 4:09 PM Yury Kotov <yury-kotov@yandex-team.ru> wrote: >> Add qtest_set_expected_status function to set expected exit status of >> child process. By default expected exit status is 0. >> >> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru> >> --- >> tests/libqtest.c | 14 +++++++++++--- >> tests/libqtest.h | 9 +++++++++ >> 2 files changed, 20 insertions(+), 3 deletions(-) > > I sent a vary similar patch with v1 of dbus-vmstate, and dropped it > because it no longer needs it in v2 (for now) "tests: add > qtest_set_exit_status()". > > Your function is better named already. > Thanks, I'll look at this realization >> diff --git a/tests/libqtest.c b/tests/libqtest.c >> index 2713b86cf7..118d779c1b 100644 >> --- a/tests/libqtest.c >> +++ b/tests/libqtest.c >> @@ -43,6 +43,7 @@ struct QTestState >> int qmp_fd; >> pid_t qemu_pid; /* our child QEMU process */ >> int wstatus; >> + int expected_status; >> bool big_endian; >> bool irq_level[MAX_IRQ]; >> GString *rx; >> @@ -113,6 +114,11 @@ bool qtest_probe_child(QTestState *s) >> return false; >> } >> >> +void qtest_set_expected_status(QTestState *s, int status) >> +{ >> + s->expected_status = status; >> +} >> + >> static void kill_qemu(QTestState *s) >> { >> pid_t pid = s->qemu_pid; >> @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s) >> * fishy and should be logged with as much detail as possible. >> */ >> wstatus = s->wstatus; >> - if (wstatus) { >> + if (WEXITSTATUS(wstatus) != s->expected_status) { > > Shouldn't it check WEXITSTATUS value only when WIFEXITED ? > Oh, it seems that you're right. I'll fix it in v2 >> if (WIFEXITED(wstatus)) { >> fprintf(stderr, "%s:%d: kill_qemu() tried to terminate QEMU " >> - "process but encountered exit status %d\n", >> - __FILE__, __LINE__, WEXITSTATUS(wstatus)); >> + "process but encountered exit status %d (expected %d)\n", >> + __FILE__, __LINE__, WEXITSTATUS(wstatus), >> + s->expected_status); >> } else if (WIFSIGNALED(wstatus)) { >> int sig = WTERMSIG(wstatus); >> const char *signame = strsignal(sig) ?: "unknown ???"; >> @@ -248,6 +255,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) >> g_test_message("starting QEMU: %s", command); >> >> s->wstatus = 0; >> + s->expected_status = 0; >> s->qemu_pid = fork(); >> if (s->qemu_pid == 0) { >> setenv("QEMU_AUDIO_DRV", "none", true); >> diff --git a/tests/libqtest.h b/tests/libqtest.h >> index 07ea35867c..c00bca94af 100644 >> --- a/tests/libqtest.h >> +++ b/tests/libqtest.h >> @@ -997,4 +997,13 @@ void qmp_assert_error_class(QDict *rsp, const char *class); >> */ >> bool qtest_probe_child(QTestState *s); >> >> +/** >> + * qtest_set_expected_status: >> + * @s: QTestState instance to operate on. >> + * @status: an expected exit status. >> + * >> + * Set expected exit status of the child. >> + */ >> +void qtest_set_expected_status(QTestState *s, int status); >> + >> #endif >> -- >> 2.22.0 > > -- > Marc-André Lureau Regards, Yury
27.08.2019, 17:04, "Eric Blake" <eblake@redhat.com>: > On 8/27/19 7:02 AM, Yury Kotov wrote: > > In the subject: 'Allow to $verb' is not idiomatic; either 'Allow > $subject to $verb' or 'Allow ${verb}ing' sound better. In this case, > I'd go with: > > tests/libqtest: Allow setting expected exit status > Ok, thanks. I'll fix it in v2 >> Add qtest_set_expected_status function to set expected exit status of >> child process. By default expected exit status is 0. > >> @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s) >> * fishy and should be logged with as much detail as possible. >> */ >> wstatus = s->wstatus; >> - if (wstatus) { >> + if (WEXITSTATUS(wstatus) != s->expected_status) { >> if (WIFEXITED(wstatus)) { > > Wrong ordering. WEXITSTATUS() is not reliable unless WIFEXITED() is true. > Yes, it's a bug.. I'll fix it in v2 > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org Regards, Yury
diff --git a/tests/libqtest.c b/tests/libqtest.c index 2713b86cf7..118d779c1b 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -43,6 +43,7 @@ struct QTestState int qmp_fd; pid_t qemu_pid; /* our child QEMU process */ int wstatus; + int expected_status; bool big_endian; bool irq_level[MAX_IRQ]; GString *rx; @@ -113,6 +114,11 @@ bool qtest_probe_child(QTestState *s) return false; } +void qtest_set_expected_status(QTestState *s, int status) +{ + s->expected_status = status; +} + static void kill_qemu(QTestState *s) { pid_t pid = s->qemu_pid; @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s) * fishy and should be logged with as much detail as possible. */ wstatus = s->wstatus; - if (wstatus) { + if (WEXITSTATUS(wstatus) != s->expected_status) { if (WIFEXITED(wstatus)) { fprintf(stderr, "%s:%d: kill_qemu() tried to terminate QEMU " - "process but encountered exit status %d\n", - __FILE__, __LINE__, WEXITSTATUS(wstatus)); + "process but encountered exit status %d (expected %d)\n", + __FILE__, __LINE__, WEXITSTATUS(wstatus), + s->expected_status); } else if (WIFSIGNALED(wstatus)) { int sig = WTERMSIG(wstatus); const char *signame = strsignal(sig) ?: "unknown ???"; @@ -248,6 +255,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) g_test_message("starting QEMU: %s", command); s->wstatus = 0; + s->expected_status = 0; s->qemu_pid = fork(); if (s->qemu_pid == 0) { setenv("QEMU_AUDIO_DRV", "none", true); diff --git a/tests/libqtest.h b/tests/libqtest.h index 07ea35867c..c00bca94af 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -997,4 +997,13 @@ void qmp_assert_error_class(QDict *rsp, const char *class); */ bool qtest_probe_child(QTestState *s); +/** + * qtest_set_expected_status: + * @s: QTestState instance to operate on. + * @status: an expected exit status. + * + * Set expected exit status of the child. + */ +void qtest_set_expected_status(QTestState *s, int status); + #endif
Add qtest_set_expected_status function to set expected exit status of child process. By default expected exit status is 0. Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru> --- tests/libqtest.c | 14 +++++++++++--- tests/libqtest.h | 9 +++++++++ 2 files changed, 20 insertions(+), 3 deletions(-)