qtest: fix "device_del" out-of-order events
cover qtest: fix "device_del" out-of-order events
It starts from a "make check" failure on one of my private tree. The problem is that when we do "device_del" we normally looking for two
things: one response (which is mostly empty), and a REMOVE event. The tricky point is the event can either be there before/after the empty response. So I added qmp_device_del() to make sure the order does not matter, then use it where proper.
Since I'm at it, I also added the sister helper qmp_device_add(), it helps to remove LOCs.
I still don't 100% sure why my private tree can trigger this error, while the master cannot. Anyway, I think this is something we should have, no matter what.
Please review. Thanks.
Peter Xu (4):
libqtest: add qmp_device_del()
tests: use qmp_device_del() where proper
libqtest: add qmp_device_add()
tests: use qmp_device_add() where proper
tests/libqos/pci.c | 15 +++-------- tests/libqos/usb.c | 28 +++++---------------- tests/libqtest.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++ tests/libqtest.h | 17 +++++++++++++ tests/usb-hcd-uhci-test.c | 28 +++------------------ tests/usb-hcd-xhci-test.c | 52 ++++---------------------------------- 6 files changed, 97 insertions(+), 106 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/4 libqtest: add qmp_device_del()
Device deletion is tricky since we'll get both a response and an event, while the order of arrival may vary. Provide a helper to handle this complexity.
--- tests/libqtest.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/libqtest.h | 8 ++++++++ 2 files changed, 56 insertions(+)
tests/libqtest.c +48 -0
@@ -925,6 +925,54 @@ QDict *qmp(const char *fmt, ...)return response; } +void qmp_device_del (const char *id) +{ +QDict *response1, *response2, *event =NULL ; +char *cmd; + +/* +* device deletion will get one response and one event. E.g.: +* +* {'execute': 'device_del','arguments': { 'id': 'scsi-hd'}} +* +* will get this one: +* +* {"timestamp": {"seconds": 1505289667, "microseconds": 569862}, +* "event": "DEVICE_DELETED", "data": {"device": "scsi-hd", +* "path": "/machine/peripheral/scsi-hd"}} +* +* and this one: +* +* {"return": {}} +* +* But the order of arrival may vary. Detect both. +*/ + + cmd =g_strdup_printf ("{'execute': 'device_del'," +" 'arguments': {" +" 'id': '%s'" +"}}" , id); + response1 =qmp (cmd); +g_free (cmd); +g_assert (response1); +g_assert (!qdict_haskey (response1,"error" )); + + response2 =qmp ("" ); +g_assert (response2); +g_assert (!qdict_haskey (response2,"error" )); + +if (qdict_haskey (response1,"event" )) { + event = response1; + }else if (qdict_haskey (response2,"event" )) { + event = response2; + } +g_assert (event); +g_assert (!strcmp (qdict_get_str (event,"event" ),"DEVICE_DELETED" )); + +QDECREF (response1); +QDECREF (response2); +} +void qmp_async (const char *fmt, ...) {va_list ap;
tests/libqtest.h +8 -0
@@ -920,6 +920,14 @@ QDict *qmp_fdv(int fd, const char *fmt, va_list ap);QDict *qmp_fd (int fd,const char *fmt, ...);/** +* qmp_device_del: +* @id: The device ID to be deleted +* +* Delete the device with ID @id from QMP interface. +*/ +void qmp_device_del (const char *id); + +/** * qtest_cb_for_every_machine: * @cb: Pointer to the callback function *
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH 1/4] libqtest: add qmp_device_del()
Quoted 43 lines · Peter Xu
Device deletion is tricky since we'll get both a response and an event, while the order of arrival may vary. Provide a helper to handle this complexity.
Signed-off-by Peter Xu <[email protected]>--- tests/libqtest.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/libqtest.h | 8 ++++++++ 2 files changed, 56 insertions(+)tests/libqtest.c +25
-0@@ -925,6 +925,54 @@ QDict *qmp(const char *fmt, ...)return response; } +void qmp_device_del (const char *id) +{ +QDict *response1, *response2, *event =NULL ; +char *cmd; + +/* +* device deletion will get one response and one event. E.g.: +* +* {'execute': 'device_del','arguments': { 'id': 'scsi-hd'}} +* +* will get this one: +* +* {"timestamp": {"seconds": 1505289667, "microseconds": 569862}, +* "event": "DEVICE_DELETED", "data": {"device": "scsi-hd", +* "path": "/machine/peripheral/scsi-hd"}} +* +* and this one: +* +* {"return": {}} +* +* But the order of arrival may vary. Detect both. +*/ + + cmd =g_strdup_printf ("{'execute': 'device_del'," + " 'arguments': {"
tests/libqtest.c QDict *qmp(const char *fmt, ...) · line 962
Quoted 10 lines
tests/libqtest.c +10
-0+" 'id': '%s'" + "}}", id); + response1 =qmp (cmd); +g_free (cmd); +g_assert (response1); +g_assert (!qdict_haskey (response1,"error" )); + + response2 =qmp ("" ); +g_assert (response2); +g_assert (!qdict_haskey (response2,"error" ));
qmp_receive() would be cleaner than qmp("").
tests/libqtest.c QDict *qmp(const char *fmt, ...) · line 975
Quoted 13 lines
tests/libqtest.c +13
-0+ +if (qdict_haskey (response1,"event" )) { + event = response1; + }else if (qdict_haskey (response2,"event" )) { + event = response2; + } +g_assert (event); +g_assert (!strcmp (qdict_get_str (event,"event" ),"DEVICE_DELETED" )); + +QDECREF (response1); +QDECREF (response2); +} +
Uh, this looks similar to existing qtest_qmp_device_del(). Do we need both?
Quoted 22 lines
tests/libqtest.c +0
-0void qmp_async (const char *fmt, ...) {va_list ap;tests/libqtest.h +8
-0@@ -920,6 +920,14 @@ QDict *qmp_fdv(int fd, const char *fmt, va_list ap);QDict *qmp_fd (int fd,const char *fmt, ...);/** +* qmp_device_del: +* @id: The device ID to be deleted +* +* Delete the device with ID @id from QMP interface. +*/ +void qmp_device_del (const char *id); + +/** * qtest_cb_for_every_machine: * @cb: Pointer to the callback function *
Re: [Qemu-devel] [PATCH 1/4] libqtest: add qmp_device_del()
Quoted 29 lines · Markus Armbruster
Quoted 15 lines · Peter Xu
Device deletion is tricky since we'll get both a response and an event, while the order of arrival may vary. Provide a helper to handle this complexity.
Signed-off-by Peter Xu <[email protected]>--- tests/libqtest.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/libqtest.h | 8 ++++++++ 2 files changed, 56 insertions(+)tests/libqtest.c +0
-0tests/libqtest.c QDict *qmp(const char *fmt, ...) · line 933
Quoted 9 lines
tests/libqtest.c +5
-0@@ -925,6 +925,54 @@ QDict *qmp(const char *fmt, ...)return response; } +void qmp_device_del (const char *id) + + response2 =qmp ("" ); +g_assert (response2); +g_assert (!qdict_haskey (response2,"error" ));
qmp_receive()would be cleaner thanqmp("").
Quoted 15 lines
tests/libqtest.c QDict *qmp(const char *fmt, ...) · line 975
Quoted 13 lines
tests/libqtest.c +13
-0+ +if (qdict_haskey (response1,"event" )) { + event = response1; + }else if (qdict_haskey (response2,"event" )) { + event = response2; + } +g_assert (event); +g_assert (!strcmp (qdict_get_str (event,"event" ),"DEVICE_DELETED" )); + +QDECREF (response1); +QDECREF (response2); +} +Uh, this looks similar to existing
qtest_qmp_device_del(). Do we need both?
In fact, if I'm reading correctly, this is an early version, which was retitled into commit acd80015 adding qtest_qmp_device_del().
At any rate, when I post my respin of my libqtest cleanups, I have the qmp("") slated to be fixed.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 2/4 tests: use qmp_device_del() where proper
In reply to Peter Xu--- tests/libqos/usb.c | 11 ++--------- tests/usb-hcd-uhci-test.c | 14 +------------- tests/usb-hcd-xhci-test.c | 29 ++--------------------------- 3 files changed, 5 insertions(+), 49 deletions(-)
tests/libqos/usb.c +2 -9
@@ -60,14 +60,7 @@ void usb_test_hotplug(const char *hcd_id, const int port,port_check (); } - cmd =g_strdup_printf ("{'execute': 'device_del'," -" 'arguments': {" -" 'id': 'usbdev%d'" -"}}" , port); - response =qmp (cmd); + cmd =g_strdup_printf ("usbdev%d" , port); +qmp_device_del (cmd);g_free (cmd); -g_assert (response); -g_assert (qdict_haskey (response,"event" )); -g_assert (!strcmp (qdict_get_str (response,"event" ),"DEVICE_DELETED" )); -QDECREF (response); }
tests/usb-hcd-uhci-test.c +1 -13
@@ -60,19 +60,7 @@ static void test_usb_storage_hotplug(void)g_assert (!qdict_haskey (response,"error" ));QDECREF (response); - response =qmp ("{'execute': 'device_del'," -" 'arguments': {" -" 'id': 'usbdev0'" -"}}" ); -g_assert (response); -g_assert (!qdict_haskey (response,"error" )); -QDECREF (response); - - response =qmp ("" ); -g_assert (response); -g_assert (qdict_haskey (response,"event" )); -g_assert (!strcmp (qdict_get_str (response,"event" ),"DEVICE_DELETED" )); -QDECREF (response); +qmp_device_del ("usbdev0" ); }int main (int argc,char **argv)
tests/usb-hcd-xhci-test.c +2 -27
@@ -49,33 +49,8 @@ static void test_usb_uas_hotplug(void)added disk is visible after BUS rescan */ - response =qmp ("{'execute': 'device_del'," -" 'arguments': {" -" 'id': 'scsi-hd'" -"}}" ); -g_assert (response); -g_assert (!qdict_haskey (response,"error" )); -QDECREF (response); - - response =qmp ("" ); -g_assert (qdict_haskey (response,"event" )); -g_assert (!strcmp (qdict_get_str (response,"event" ),"DEVICE_DELETED" )); -QDECREF (response); - - - response =qmp ("{'execute': 'device_del'," -" 'arguments': {" -" 'id': 'uas'" -"}}" ); -g_assert (response); -g_assert (!qdict_haskey (response,"error" )); -QDECREF (response); - - response =qmp ("" ); -g_assert (response); -g_assert (qdict_haskey (response,"event" )); -g_assert (!strcmp (qdict_get_str (response,"event" ),"DEVICE_DELETED" )); -QDECREF (response); +qmp_device_del ("scsi-hd" ); +qmp_device_del ("uas" ); }int main (int argc,char **argv)
Signature · 1 line
2.7.4
PATCH 3/4 libqtest: add qmp_device_add()
In reply to Peter XuSince we have qmp_device_del(), pair them up.
--- tests/libqtest.c | 15 +++++++++++++++ tests/libqtest.h | 9 +++++++++ 2 files changed, 24 insertions(+)
tests/libqtest.c +15 -0
@@ -925,6 +925,21 @@ QDict *qmp(const char *fmt, ...)return response; } +void qmp_device_add (const char *args) +{ +QDict *response; +char *cmd; + + cmd =g_strdup_printf ("{'execute': 'device_add'," +" 'arguments': { %s }" +"}" , args); + response =qmp (cmd); +g_free (cmd); +g_assert (response); +g_assert (!qdict_haskey (response,"error" )); +QDECREF (response); +} +void qmp_device_del (const char *id) {QDict *response1, *response2, *event =NULL ;
tests/libqtest.h +9 -0
@@ -920,6 +920,15 @@ QDict *qmp_fdv(int fd, const char *fmt, va_list ap);QDict *qmp_fd (int fd,const char *fmt, ...);/** +* qmp_device_add: +* @args: Parameters for the new device, like: +* "'driver': 'XXX', 'id': 'XXX', 'addr': 'XXX'" +* +* Create a new device with parameter @args provided. +*/ +void qmp_device_add (const char *args); + +/** * qmp_device_del: * @id: The device ID to be deleted *
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH 3/4] libqtest: add qmp_device_add()
Quoted 13 lines · Peter Xu
Since we have
qmp_device_del(), pair them up.Signed-off-by Peter Xu <[email protected]>--- tests/libqtest.c | 15 +++++++++++++++ tests/libqtest.h | 9 +++++++++ 2 files changed, 24 insertions(+)tests/libqtest.c +0
-0
tests/libqtest.c QDict *qmp(const char *fmt, ...) · line 928
Quoted 4 lines
tests/libqtest.c +1
-0@@ -925,6 +925,21 @@ QDict *qmp(const char *fmt, ...)return response; } +void qmp_device_add (const char *args)
I think it would be nicer to have a function with variable args here, so that the callers do not have to do the g_strdup_printf() dance all over the place. See e.g. my qtest_hot_plug_device() function in my patch
here: http://patchwork.ozlabs.org/patch/801487/
If you fix that, I don't mind if we finally go with my patch or with yours... If we decide to go with my patch, it would be nice to get some Reviewed-bys for it. I think I could then send a PULL request for it, since I've got some other test related patches in my queue and we do not have a real maintainer for the tests directory...
Thomas
PATCH 4/4 tests: use qmp_device_add() where proper
In reply to Peter Xu--- tests/libqos/pci.c | 15 +++------------ tests/libqos/usb.c | 17 ++++------------- tests/usb-hcd-uhci-test.c | 14 ++------------ tests/usb-hcd-xhci-test.c | 23 +++-------------------- 4 files changed, 12 insertions(+), 57 deletions(-)
tests/libqos/pci.c +3 -12
@@ -394,21 +394,12 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)void qpci_plug_device_test (const char *driver,const char *id,uint8_t slot,const char *opts) { -QDict *response;char *cmd; - cmd =g_strdup_printf ("{'execute': 'device_add'," -" 'arguments': {" -" 'driver': '%s'," -" 'addr': '%d'," -" %s%s" -" 'id': '%s'" -"}}" , driver, slot, + cmd =g_strdup_printf ("'driver': '%s', 'addr': '%d'," +"%s%s 'id': '%s'" , driver, slot, opts ? opts :"" , opts ?"," :"" , id); - response =qmp (cmd); +qmp_device_add (cmd);g_free (cmd); -g_assert (response); -g_assert (!qdict_haskey (response,"error" )); -QDECREF (response); }
tests/libqos/usb.c +4 -13
@@ -40,21 +40,12 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)void usb_test_hotplug (const char *hcd_id,const int port,void (*port_check)(void )) { -QDict *response;char *cmd; - cmd =g_strdup_printf ("{'execute': 'device_add'," -" 'arguments': {" -" 'driver': 'usb-tablet'," -" 'port': '%d'," -" 'bus': '%s.0'," -" 'id': 'usbdev%d'" -"}}" , port, hcd_id, port); - response =qmp (cmd); -g_free (cmd); -g_assert (response); -g_assert (!qdict_haskey (response,"error" )); -QDECREF (response); + cmd =g_strdup_printf ("'driver': 'usb-tablet', 'port': '%d'," +"'bus': '%s.0', 'id': 'usbdev%d'" , + port, hcd_id, port); +qmp_device_add (cmd);if (port_check) {port_check ();
tests/usb-hcd-uhci-test.c +2 -12
@@ -48,18 +48,8 @@ static void test_uhci_hotplug(void)static void test_usb_storage_hotplug (void ) { -QDict *response; - - response =qmp ("{'execute': 'device_add'," -" 'arguments': {" -" 'driver': 'usb-storage'," -" 'drive': 'drive0'," -" 'id': 'usbdev0'" -"}}" ); -g_assert (response); -g_assert (!qdict_haskey (response,"error" )); -QDECREF (response); - +qmp_device_add ("'driver': 'usb-storage'," +"'drive': 'drive0', 'id': 'usbdev0'" );qmp_device_del ("usbdev0" ); }
tests/usb-hcd-xhci-test.c +3 -20
@@ -23,26 +23,9 @@ static void test_xhci_hotplug(void)static void test_usb_uas_hotplug (void ) { -QDict *response; - - response =qmp ("{'execute': 'device_add'," -" 'arguments': {" -" 'driver': 'usb-uas'," -" 'id': 'uas'" -"}}" ); -g_assert (response); -g_assert (!qdict_haskey (response,"error" )); -QDECREF (response); - - response =qmp ("{'execute': 'device_add'," -" 'arguments': {" -" 'driver': 'scsi-hd'," -" 'drive': 'drive0'," -" 'id': 'scsi-hd'" -"}}" ); -g_assert (response); -g_assert (!qdict_haskey (response,"error" )); -QDECREF (response); +qmp_device_add ("'driver': 'usb-uas', 'id': 'uas'" ); +qmp_device_add ("'driver': 'scsi-hd', 'drive': 'drive0'," +"'id': 'scsi-hd'" );/* TODO: UAS HBA driver in libqos, to check that
Signature · 1 line
2.7.4
Quoted 9 lines · Peter Xu
It starts from a "make check" failure on one of my private tree. The problem is that when we do "
device_del" we normally looking for two
things: one response (which is mostly empty), and a REMOVE event. The tricky point is the event can either be there before/after the empty response. So I addedqmp_device_del()to make sure the order does not matter, then use it where proper.Since I'm at it, I also added the sister helper
qmp_device_add(), it helps to remove LOCs.
I've had a similar idea a couple of weeks ago, see here:
http://patchwork.ozlabs.org/patch/801487/
Quoted 3 lines
I still don't 100% sure why my private tree can trigger this error, while the master cannot. Anyway, I think this is something we should have, no matter what.
Did you maybe touch the USB tests in your private tree?
As far as I know, some test currently use QMP in a bad way, for example usb_test_hotplug() only checks for the DEVICE_DELETED at the end, but forgets to read back the final return value. That return value is then presented to the next part of the code that uses QMP instead ... it currently only works more or less by accident, but as soon as you try to add new code inbetween, it certainly will fail.
==> We really got to clean this up (either with my patch or your patch series).
Thomas
Quoted 8 lines · Thomas Huth
Quoted 7 lines · Peter Xu
It starts from a "make check" failure on one of my private tree. The problem is that when we do "
device_del" we normally looking for two
things: one response (which is mostly empty), and a REMOVE event. The tricky point is the event can either be there before/after the empty response. So I addedqmp_device_del()to make sure the order does not matter, then use it where proper.
Quoted 6 lines
Quoted 2 lines
Since I'm at it, I also added the sister helper
qmp_device_add(), it helps to remove LOCs.I've had a similar idea a couple of weeks ago, see here:
Good to know!
Quoted 6 lines
Quoted 4 lines
I still don't 100% sure why my private tree can trigger this error, while the master cannot. Anyway, I think this is something we should have, no matter what.
Did you maybe touch the USB tests in your private tree?
No. But I changed some internals of QMP there, I guess that's the reason that caused the misorder to happen more easily.
Quoted 8 lines
As far as I know, some test currently use QMP in a bad way, for example
usb_test_hotplug()only checks for theDEVICE_DELETEDat the end, but forgets to read back the final return value. That return value is then presented to the next part of the code that uses QMP instead ... it currently only works more or less by accident, but as soon as you try to add new code inbetween, it certainly will fail.
==> We really got to clean this up (either with my patch or your patch series).
Agree.
I think your patch is nicer on the interface (as you have mentioned in the other reply), I can try to review it later.
However it seems that your patch didn't really solve the problem I encountered (mis-ordered message arrivals). It would be good if you want to solve it together, or I can draft patch upon yours.
Thanks,
Signature · 1 line
Peter Xu
Quoted 37 lines · Peter Xu
Quoted 8 lines · Thomas Huth
Quoted 7 lines · Peter Xu
It starts from a "make check" failure on one of my private tree. The problem is that when we do "
device_del" we normally looking for two
things: one response (which is mostly empty), and a REMOVE event. The tricky point is the event can either be there before/after the empty response. So I addedqmp_device_del()to make sure the order does not matter, then use it where proper.Quoted 6 lines
Quoted 2 lines
Since I'm at it, I also added the sister helper
qmp_device_add(), it helps to remove LOCs.I've had a similar idea a couple of weeks ago, see here:
Good to know!
Quoted 6 lines
Quoted 4 lines
I still don't 100% sure why my private tree can trigger this error, while the master cannot. Anyway, I think this is something we should have, no matter what.
Did you maybe touch the USB tests in your private tree?
No. But I changed some internals of QMP there, I guess that's the reason that caused the misorder to happen more easily.
Quoted 8 lines
As far as I know, some test currently use QMP in a bad way, for example
usb_test_hotplug()only checks for theDEVICE_DELETEDat the end, but forgets to read back the final return value. That return value is then presented to the next part of the code that uses QMP instead ... it currently only works more or less by accident, but as soon as you try to add new code inbetween, it certainly will fail.
==> We really got to clean this up (either with my patch or your patch series).
Quoted 8 lines
Agree.
I think your patch is nicer on the interface (as you have mentioned in the other reply), I can try to review it later.
However it seems that your patch didn't really solve the problem I encountered (mis-ordered message arrivals). It would be good if you want to solve it together, or I can draft patch upon yours.
True, I'll try to respin my patch, including the ideas from your patch...
Thomas
On Wed, Sep 13, 2017 at 12:35:17PM +0200, Thomas Huth wrote:
[...]
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
As far as I know, some test currently use QMP in a bad way, for example
usb_test_hotplug()only checks for theDEVICE_DELETEDat the end, but forgets to read back the final return value. That return value is then presented to the next part of the code that uses QMP instead ... it currently only works more or less by accident, but as soon as you try to add new code inbetween, it certainly will fail.
==> We really got to clean this up (either with my patch or your patch series).Agree.
Quoted 8 lines
Quoted 6 lines
I think your patch is nicer on the interface (as you have mentioned in the other reply), I can try to review it later.
However it seems that your patch didn't really solve the problem I encountered (mis-ordered message arrivals). It would be good if you want to solve it together, or I can draft patch upon yours.
True, I'll try to respin my patch, including the ideas from your patch...
That would be great. I've left some comments there already. Thanks!
Signature · 1 line
Peter Xu