[v1] UUID validation during migration

[Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

Posted by Yury Kotov 6 years, 2 months ago

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(-)

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

Re: [Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

Posted by Marc-André Lureau 6 years, 2 months ago

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


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

Posted by Yury Kotov 6 years, 2 months ago

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

Re: [Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

Posted by Eric Blake 6 years, 2 months ago

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

Posted by Yury Kotov 6 years, 2 months ago

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