Re: [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering

[Thomas Huth]

On 21/08/2019 17.53, Daniel P. Berrangé wrote:
> The ordering of events that are emitted during the rmdir
> test have changed with kernel >= 5.3. Semantically both
> new & old orderings are correct, so we must be able to
> cope with either.
> 
> To cope with this, when we see an unexpected event, we
> push it back onto the queue and look and the subsequent
> event to see if that matches instead.
> 
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>  tests/test-util-filemonitor.c | 43 +++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
> index 46e781c022..301cd2db61 100644
> --- a/tests/test-util-filemonitor.c
> +++ b/tests/test-util-filemonitor.c
> @@ -45,6 +45,11 @@ typedef struct {
>      const char *filedst;
>      int64_t *watchid;
>      int eventid;
> +    /*
> +     * Only valid with OP_EVENT - this event might be
> +     * swapped with the next OP_EVENT
> +     */
> +    bool swapnext;
>  } QFileMonitorTestOp;
>  
>  typedef struct {
> @@ -98,6 +103,10 @@ qemu_file_monitor_test_handler(int64_t id,
>      QFileMonitorTestData *data = opaque;
>      QFileMonitorTestRecord *rec = g_new0(QFileMonitorTestRecord, 1);
>  
> +    if (debug) {
> +        g_printerr("Queue event id %" PRIx64 " event %d file %s\n",
> +                   id, event, filename);
> +    }
>      rec->id = id;
>      rec->event = event;
>      rec->filename = g_strdup(filename);
> @@ -125,7 +134,8 @@ qemu_file_monitor_test_record_free(QFileMonitorTestRecord 
> *rec)
>   * to wait for the event to be queued for us.
>   */
>  static QFileMonitorTestRecord *
> -qemu_file_monitor_test_next_record(QFileMonitorTestData *data)
> +qemu_file_monitor_test_next_record(QFileMonitorTestData *data,
> +                                   QFileMonitorTestRecord *pushback)
>  {
>      GTimer *timer = g_timer_new();
>      QFileMonitorTestRecord *record = NULL;
> @@ -139,9 +149,15 @@ qemu_file_monitor_test_next_record(QFileMonitorTestData 
> *data)
>      }
>      if (data->records) {
>          record = data->records->data;
> -        tmp = data->records;
> -        data->records = g_list_remove_link(data->records, tmp);
> -        g_list_free(tmp);
> +        if (pushback) {
> +            data->records->data = pushback;
> +        } else {
> +            tmp = data->records;
> +            data->records = g_list_remove_link(data->records, tmp);
> +            g_list_free(tmp);
> +        }
> +    } else if (pushback) {
> +        qemu_file_monitor_test_record_free(pushback);
>      }
>      qemu_mutex_unlock(&data->lock);
>  
> @@ -158,13 +174,15 @@ static bool
>  qemu_file_monitor_test_expect(QFileMonitorTestData *data,
>                                int64_t id,
>                                QFileMonitorEvent event,
> -                              const char *filename)
> +                              const char *filename,
> +                              bool swapnext)
>  {
>      QFileMonitorTestRecord *rec;
>      bool ret = false;
>  
> -    rec = qemu_file_monitor_test_next_record(data);
> +    rec = qemu_file_monitor_test_next_record(data, NULL);
>  
> + retry:
>      if (!rec) {
>          g_printerr("Missing event watch id %" PRIx64 " event %d file %s\n",
>                     id, event, filename);
> @@ -172,6 +190,11 @@ qemu_file_monitor_test_expect(QFileMonitorTestData *data,
>      }
>  
>      if (id != rec->id) {
> +        if (swapnext) {
> +            rec = qemu_file_monitor_test_next_record(data, rec);
> +            swapnext = false;
> +            goto retry;
> +        }
>          g_printerr("Expected watch id %" PRIx64 " but got %" PRIx64 "\n",
>                     id, rec->id);
>          goto cleanup;
> @@ -347,7 +370,8 @@ test_file_monitor_events(void)
>            .filesrc = "fish", },
>          { .type = QFILE_MONITOR_TEST_OP_EVENT,
>            .filesrc = "", .watchid = &watch4,
> -          .eventid = QFILE_MONITOR_EVENT_IGNORED },
> +          .eventid = QFILE_MONITOR_EVENT_IGNORED,
> +          .swapnext = true },
>          { .type = QFILE_MONITOR_TEST_OP_EVENT,
>            .filesrc = "fish", .watchid = &watch0,
>            .eventid = QFILE_MONITOR_EVENT_DELETED },
> @@ -493,8 +517,9 @@ test_file_monitor_events(void)
>                  g_printerr("Event id=%" PRIx64 " event=%d file=%s\n",
>                             *op->watchid, op->eventid, op->filesrc);
>              }
> -            if (!qemu_file_monitor_test_expect(
> -                    &data, *op->watchid, op->eventid, op->filesrc))
> +            if (!qemu_file_monitor_test_expect(&data, *op->watchid,
> +                                               op->eventid, op->filesrc,
> +                                               op->swapnext))
>                  goto cleanup;
>              break;
>          case QFILE_MONITOR_TEST_OP_CREATE:
> 

I don't know that part of the code, but this looks ok to me at a quick
glance. So FWIW, from my side a light
Reviewed-by: Thomas Huth <address@hidden>