* Re: [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering
2019-08-21 15:53 [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering Daniel P. Berrangé
@ 2019-08-22 0:35 ` Wei Yang
2019-08-22 3:37 ` Peter Xu
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Wei Yang @ 2019-08-22 0:35 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Peter Xu, Wei Yang
On Wed, Aug 21, 2019 at 04:53:27PM +0100, 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é <berrange@redhat.com>
Tested-by: Wei Yang <richardw.yang@linux.intel.com>
>---
> 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:
>--
>2.21.0
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering
2019-08-21 15:53 [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering Daniel P. Berrangé
2019-08-22 0:35 ` Wei Yang
@ 2019-08-22 3:37 ` Peter Xu
2019-08-22 16:21 ` Cornelia Huck
2019-09-04 10:10 ` Thomas Huth
3 siblings, 0 replies; 5+ messages in thread
From: Peter Xu @ 2019-08-22 3:37 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Wei Yang
On Wed, Aug 21, 2019 at 04:53:27PM +0100, 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é <berrange@redhat.com>
Thanks for fixing it!
Tested-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering
2019-08-21 15:53 [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering Daniel P. Berrangé
2019-08-22 0:35 ` Wei Yang
2019-08-22 3:37 ` Peter Xu
@ 2019-08-22 16:21 ` Cornelia Huck
2019-09-04 10:10 ` Thomas Huth
3 siblings, 0 replies; 5+ messages in thread
From: Cornelia Huck @ 2019-08-22 16:21 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Peter Xu, Wei Yang
On Wed, 21 Aug 2019 16:53:27 +0100
Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
> ---
> tests/test-util-filemonitor.c | 43 +++++++++++++++++++++++++++--------
> 1 file changed, 34 insertions(+), 9 deletions(-)
Tested-by: Cornelia Huck <cohuck@redhat.com>
I tried to review this patch, but I fear I'm out of my depth here :(
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering
2019-08-21 15:53 [Qemu-devel] [PATCH] tests: make filemonitor test more robust to event ordering Daniel P. Berrangé
` (2 preceding siblings ...)
2019-08-22 16:21 ` Cornelia Huck
@ 2019-09-04 10:10 ` Thomas Huth
3 siblings, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2019-09-04 10:10 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Wei Yang, Peter Xu
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é <berrange@redhat.com>
> ---
> 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 <thuth@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread