| Message ID | 20201006123904.610658-3-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand |
On 06/10/2020 14.38, Maxim Levitsky wrote: > The new qtest_qmp_receive buffers all the received qmp events, allowing > qtest_qmp_eventwait_ref to return them. > > This is intended to solve the race in regard to ordering of qmp events > vs qmp responses, as soon as the callers start using the new interface. > > In addition to that, define qtest_qmp_event_ref a function which only scans > the buffer that qtest_qmp_receive stores the events to. > > This is intended for callers that are only interested in events that were > received during the last call to the qtest_qmp_receive. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > tests/qtest/libqos/libqtest.h | 23 ++++++++++++++++ > tests/qtest/libqtest.c | 49 ++++++++++++++++++++++++++++++++++- > 2 files changed, 71 insertions(+), 1 deletion(-) > > diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h > index a41135fc92..19429a536d 100644 > --- a/tests/qtest/libqos/libqtest.h > +++ b/tests/qtest/libqos/libqtest.h > @@ -198,6 +198,16 @@ void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap) > */ > QDict *qtest_qmp_receive_dict(QTestState *s); > > +/** > + * qtest_qmp_receive: > + * @s: #QTestState instance to operate on. > + * > + * Reads a QMP message from QEMU and returns the response. > + * Buffers all the events received meanwhile, until a > + * call to qtest_qmp_eventwait > + */ > +QDict *qtest_qmp_receive(QTestState *s); Re-introducing qtest_qmp_receive() with different behavior than before will likely make backports of other later patches a pain, and might also break other patches that use this function but are not merged yet. Could you please use a different name for this function instead? Maye qtest_qmp_receive_buffered() or something like that? Thomas
On 12/10/20 13:14, Thomas Huth wrote: >> +/** >> + * qtest_qmp_receive: >> + * @s: #QTestState instance to operate on. >> + * >> + * Reads a QMP message from QEMU and returns the response. >> + * Buffers all the events received meanwhile, until a >> + * call to qtest_qmp_eventwait >> + */ >> +QDict *qtest_qmp_receive(QTestState *s); > Re-introducing qtest_qmp_receive() with different behavior than before will > likely make backports of other later patches a pain, and might also break > other patches that use this function but are not merged yet. Could you > please use a different name for this function instead? Maye > qtest_qmp_receive_buffered() or something like that? We chose to use the same name because the new version generally is the one you want and, except for the handling of events, is exactly the same as before. In other words, I'm treating the new semantics more as a bugfix than a feature. The only trap that backports of later patches could fall into is if they want to look at events, but it would be caught easily because the test would fail. Paolo
On 12/10/2020 15.47, Paolo Bonzini wrote: > On 12/10/20 13:14, Thomas Huth wrote: >>> +/** >>> + * qtest_qmp_receive: >>> + * @s: #QTestState instance to operate on. >>> + * >>> + * Reads a QMP message from QEMU and returns the response. >>> + * Buffers all the events received meanwhile, until a >>> + * call to qtest_qmp_eventwait >>> + */ >>> +QDict *qtest_qmp_receive(QTestState *s); >> Re-introducing qtest_qmp_receive() with different behavior than before will >> likely make backports of other later patches a pain, and might also break >> other patches that use this function but are not merged yet. Could you >> please use a different name for this function instead? Maye >> qtest_qmp_receive_buffered() or something like that? > > We chose to use the same name because the new version generally is the > one you want and, except for the handling of events, is exactly the same > as before. In other words, I'm treating the new semantics more as a > bugfix than a feature. > > The only trap that backports of later patches could fall into is if they > want to look at events, but it would be caught easily because the test > would fail. Ok, thanks for the explanation! ... but I think it might be good to have this information in the patch description, though. Thomas
On 12/10/20 15:49, Thomas Huth wrote: >> We chose to use the same name because the new version generally is the >> one you want and, except for the handling of events, is exactly the same >> as before. In other words, I'm treating the new semantics more as a >> bugfix than a feature. >> >> The only trap that backports of later patches could fall into is if they >> want to look at events, but it would be caught easily because the test >> would fail. > Ok, thanks for the explanation! ... but I think it might be good to have > this information in the patch description, though. Ok, I'll add a couple words. Paolo
diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h index a41135fc92..19429a536d 100644 --- a/tests/qtest/libqos/libqtest.h +++ b/tests/qtest/libqos/libqtest.h @@ -198,6 +198,16 @@ void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap) */ QDict *qtest_qmp_receive_dict(QTestState *s); +/** + * qtest_qmp_receive: + * @s: #QTestState instance to operate on. + * + * Reads a QMP message from QEMU and returns the response. + * Buffers all the events received meanwhile, until a + * call to qtest_qmp_eventwait + */ +QDict *qtest_qmp_receive(QTestState *s); + /** * qtest_qmp_eventwait: * @s: #QTestState instance to operate on. @@ -217,6 +227,19 @@ void qtest_qmp_eventwait(QTestState *s, const char *event); */ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event); +/** + * qtest_qmp_event_ref: + * @s: #QTestState instance to operate on. + * @s: #event event to return. + * + * Removes non-matching events from the buffer that was set by + * qtest_qmp_receive, until an event bearing the given name is found, + * and returns it. + * If no event matches, clears the buffer and returns NULL. + * + */ +QDict *qtest_qmp_event_ref(QTestState *s, const char *event); + /** * qtest_qmp_receive_success: * @s: #QTestState instance to operate on diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index dadc465825..d4c49a52ff 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -63,6 +63,7 @@ struct QTestState bool irq_level[MAX_IRQ]; GString *rx; QTestTransportOps ops; + GList *pending_events; }; static GHookList abrt_hooks; @@ -279,6 +280,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) g_test_message("starting QEMU: %s", command); + s->pending_events = NULL; s->wstatus = 0; s->expected_status = 0; s->qemu_pid = fork(); @@ -386,6 +388,13 @@ void qtest_quit(QTestState *s) close(s->fd); close(s->qmp_fd); g_string_free(s->rx, true); + + for (GList *it = s->pending_events; it != NULL; it = it->next) { + qobject_unref((QDict *)it->data); + } + + g_list_free(s->pending_events); + g_free(s); } @@ -603,6 +612,19 @@ QDict *qmp_fd_receive(int fd) return qmp.response; } +QDict *qtest_qmp_receive(QTestState *s) +{ + while (true) { + QDict *response = qtest_qmp_receive_dict(s); + + if (!qdict_get_try_str(response, "event")) { + return response; + } + /* Stash the event for a later consumption */ + s->pending_events = g_list_prepend(s->pending_events, response); + } +} + QDict *qtest_qmp_receive_dict(QTestState *s) { return qmp_fd_receive(s->qmp_fd); @@ -771,10 +793,34 @@ void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...) va_end(ap); } -QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event) +QDict *qtest_qmp_event_ref(QTestState *s, const char *event) { + GList *next = NULL; QDict *response; + for (GList *it = s->pending_events; it != NULL; it = next) { + + next = it->next; + response = (QDict *)it->data; + + s->pending_events = g_list_remove_link(s->pending_events, it); + + if (!strcmp(qdict_get_str(response, "event"), event)) { + return response; + } + qobject_unref(response); + } + return NULL; +} + +QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event) +{ + QDict *response = qtest_qmp_event_ref(s, event); + + if (response) { + return response; + } + for (;;) { response = qtest_qmp_receive_dict(s); if ((qdict_haskey(response, "event")) && @@ -1403,6 +1449,7 @@ QTestState *qtest_inproc_init(QTestState **s, bool log, const char* arch, { QTestState *qts; qts = g_new0(QTestState, 1); + qts->pending_events = NULL; *s = qts; /* Expose qts early on, since the query endianness relies on it */ qts->wstatus = 0; for (int i = 0; i < MAX_IRQ; i++) {
The new qtest_qmp_receive buffers all the received qmp events, allowing qtest_qmp_eventwait_ref to return them. This is intended to solve the race in regard to ordering of qmp events vs qmp responses, as soon as the callers start using the new interface. In addition to that, define qtest_qmp_event_ref a function which only scans the buffer that qtest_qmp_receive stores the events to. This is intended for callers that are only interested in events that were received during the last call to the qtest_qmp_receive. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- tests/qtest/libqos/libqtest.h | 23 ++++++++++++++++ tests/qtest/libqtest.c | 49 ++++++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 1 deletion(-)