* [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test @ 2017-09-05 9:53 Thomas Huth 2017-09-05 11:42 ` Markus Armbruster 0 siblings, 1 reply; 13+ messages in thread From: Thomas Huth @ 2017-09-05 9:53 UTC (permalink / raw) To: Dr. David Alan Gilbert, qemu-devel Cc: qemu-ppc, Alexander Graf, John Snow, Igor Mammedov, Philippe Mathieu-Daudé, Eric Blake, armbru, Andreas Färber People tend to forget to mark internal devices with "user_creatable = false or hotpluggable = false, and these devices can crash QEMU if added via the HMP monitor. So let's add a test to run through all devices and that tries to add them blindly (without arguments) to see whether this could crash the QEMU instance. Signed-off-by: Thomas Huth <thuth@redhat.com> --- I've marked the patch as RFC since not all of the required device bug fixes have been merged yet (so this patch can not be included yet without breaking "make check"). It's also sad that "macio-oldworld" currently has to be blacklisted - I tried to find a fix for that device, but I was not able to spot the exact problem so far. So help for fixing that device is very very welcome! The crash can be reproduced like this: $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none QEMU 2.10.50 monitor - type 'help' for more information (qemu) device_add macio-oldworld,id=x (qemu) device_del x (qemu) ** ERROR:qom/object.c:1611:object_get_canonical_path_component: assertion failed: (obj->parent != NULL) Aborted (core dumped) tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/tests/test-hmp.c b/tests/test-hmp.c index 7a38cdc..e8a25c4 100644 --- a/tests/test-hmp.c +++ b/tests/test-hmp.c @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = { "commit all", "cpu-add 1", "cpu 0", - "device_add ?", "device_add usb-mouse,id=mouse1", "mouse_button 7", "mouse_move 10 10", @@ -116,6 +115,64 @@ static void test_info_commands(void) g_free(info_buf); } +/* + * People tend to forget to mark internal devices with "user_creatable = false + * and these devices can crash QEMU if added via the HMP monitor. So let's run + * through all devices and try to add them blindly (without arguments) to see + * whether this could crash the QEMU instance. + */ +static void test_device_add_commands(void) +{ + char *resp, *devices, *devices_buf, *endp; + + devices = devices_buf = hmp("device_add help"); + + while (*devices) { + /* Ignore header lines etc. */ + if (strncmp(devices, "name \"", 6)) { + devices = strchr(devices, '\n'); + if (!devices) { + break; + } + devices += 1; + continue; + } + /* Extract the device name, ignore parameters and description */ + devices += 6; + endp = strchr(devices, '"'); + g_assert(endp != NULL); + *endp = '\0'; + /* Check whether it is blacklisted... */ + if (g_str_equal("macio-oldworld", devices)) { + devices = strchr(endp + 1, '\n'); + if (!devices) { + break; + } + devices += 1; + continue; + } + /* Now run the device_add + device_del commands */ + if (verbose) { + fprintf(stderr, "\tdevice_add %s,id=%s\n", devices, devices); + } + resp = hmp("device_add %s,id=%s", devices, devices); + g_free(resp); + if (verbose) { + fprintf(stderr, "\tdevice_del %s\n", devices); + } + resp = hmp("device_del %s", devices); + g_free(resp); + /* And move forward to the next line */ + devices = strchr(endp + 1, '\n'); + if (!devices) { + break; + } + devices += 1; + } + + g_free(devices_buf); +} + static void test_machine(gconstpointer data) { const char *machine = data; @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data) qtest_start(args); test_info_commands(); + test_device_add_commands(); test_commands(); qtest_end(); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-05 9:53 [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test Thomas Huth @ 2017-09-05 11:42 ` Markus Armbruster 2017-09-05 16:48 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 13+ messages in thread From: Markus Armbruster @ 2017-09-05 11:42 UTC (permalink / raw) To: Thomas Huth Cc: Dr. David Alan Gilbert, qemu-devel, Philippe Mathieu-Daudé, Alexander Graf, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber Thomas Huth <thuth@redhat.com> writes: > People tend to forget to mark internal devices with "user_creatable = false > or hotpluggable = false, and these devices can crash QEMU if added via the > HMP monitor. So let's add a test to run through all devices and that tries > to add them blindly (without arguments) to see whether this could crash the > QEMU instance. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > I've marked the patch as RFC since not all of the required device bug > fixes have been merged yet (so this patch can not be included yet without > breaking "make check"). It's also sad that "macio-oldworld" currently > has to be blacklisted - I tried to find a fix for that device, but I was > not able to spot the exact problem so far. So help for fixing that device > is very very welcome! The crash can be reproduced like this: > > $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none > QEMU 2.10.50 monitor - type 'help' for more information > (qemu) device_add macio-oldworld,id=x > (qemu) device_del x > (qemu) ** > ERROR:qom/object.c:1611:object_get_canonical_path_component: > assertion failed: (obj->parent != NULL) > Aborted (core dumped) > > tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 59 insertions(+), 1 deletion(-) > > diff --git a/tests/test-hmp.c b/tests/test-hmp.c > index 7a38cdc..e8a25c4 100644 > --- a/tests/test-hmp.c > +++ b/tests/test-hmp.c > @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = { > "commit all", > "cpu-add 1", > "cpu 0", > - "device_add ?", > "device_add usb-mouse,id=mouse1", > "mouse_button 7", > "mouse_move 10 10", > @@ -116,6 +115,64 @@ static void test_info_commands(void) > g_free(info_buf); > } > > +/* > + * People tend to forget to mark internal devices with "user_creatable = false > + * and these devices can crash QEMU if added via the HMP monitor. So let's run > + * through all devices and try to add them blindly (without arguments) to see > + * whether this could crash the QEMU instance. > + */ > +static void test_device_add_commands(void) > +{ > + char *resp, *devices, *devices_buf, *endp; > + > + devices = devices_buf = hmp("device_add help"); > + > + while (*devices) { > + /* Ignore header lines etc. */ > + if (strncmp(devices, "name \"", 6)) { > + devices = strchr(devices, '\n'); > + if (!devices) { > + break; > + } > + devices += 1; > + continue; > + } > + /* Extract the device name, ignore parameters and description */ > + devices += 6; > + endp = strchr(devices, '"'); > + g_assert(endp != NULL); > + *endp = '\0'; > + /* Check whether it is blacklisted... */ > + if (g_str_equal("macio-oldworld", devices)) { > + devices = strchr(endp + 1, '\n'); > + if (!devices) { > + break; > + } > + devices += 1; > + continue; > + } > + /* Now run the device_add + device_del commands */ > + if (verbose) { > + fprintf(stderr, "\tdevice_add %s,id=%s\n", devices, devices); > + } > + resp = hmp("device_add %s,id=%s", devices, devices); > + g_free(resp); > + if (verbose) { > + fprintf(stderr, "\tdevice_del %s\n", devices); > + } > + resp = hmp("device_del %s", devices); > + g_free(resp); > + /* And move forward to the next line */ > + devices = strchr(endp + 1, '\n'); > + if (!devices) { > + break; > + } > + devices += 1; > + } > + > + g_free(devices_buf); > +} > + > static void test_machine(gconstpointer data) > { > const char *machine = data; > @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data) > qtest_start(args); > > test_info_commands(); > + test_device_add_commands(); > test_commands(); > > qtest_end(); This finds devices by parsing output of HMP help. I think you should use introspection instead, like device-introspect-test does. You may want to extract common utility code from there. The actual device_add and device_del also use HMP. Failures are ignored. A few device_add failures I'd expect: * There is no suitable bus. * There are suitable buses, but the default one is full. * Mandatory parameters are missing, such as device backend. * The bus doesn't support hot plug (some other bus might). * The device supports only cold plug with -device, not hot plug with device_add. I'm afraid the test only tests one of these common failure modes for many devices. device_del failures I'd expect: * The device doesn't exist, because it hasn't completed hot plug, yet. In some cases such as ACPI PCI hot plug, hot plug may require guest cooperation, which may take unbounded time. device_add merely kicks off the hot plug then. I can't remember how to poll for completion. I also can't remember why we don't send a QMP event. The hot plug usually completes quickly, but it may take its own sweet time, or not complete at all, say because the guest doesn't support ACPI, or just doesn't feel like plugging right now. The test needs to set up a guest that cooperates. I guess that involves libqos; yet another thing I've forgotten. * Same for device_del. You should wait for the DEVICE_DELETED event with a suitable timeout. We could improve device_add to cold plug before the machine starts, i.e. between start with -S and the first cont. We could similarly improve device_del to cold plug. Together, that would let you sidestep the hot plug complications. I guess this test is a case of "if it was easy, we would've done it already"... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-05 11:42 ` Markus Armbruster @ 2017-09-05 16:48 ` Dr. David Alan Gilbert 2017-09-05 18:11 ` [Qemu-devel] [Qemu-ppc] " Thomas Huth 0 siblings, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2017-09-05 16:48 UTC (permalink / raw) To: Markus Armbruster Cc: Thomas Huth, qemu-devel, Philippe Mathieu-Daudé, Alexander Graf, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber * Markus Armbruster (armbru@redhat.com) wrote: > Thomas Huth <thuth@redhat.com> writes: > > > People tend to forget to mark internal devices with "user_creatable = false > > or hotpluggable = false, and these devices can crash QEMU if added via the > > HMP monitor. So let's add a test to run through all devices and that tries > > to add them blindly (without arguments) to see whether this could crash the > > QEMU instance. > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > I've marked the patch as RFC since not all of the required device bug > > fixes have been merged yet (so this patch can not be included yet without > > breaking "make check"). It's also sad that "macio-oldworld" currently > > has to be blacklisted - I tried to find a fix for that device, but I was > > not able to spot the exact problem so far. So help for fixing that device > > is very very welcome! The crash can be reproduced like this: > > > > $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none > > QEMU 2.10.50 monitor - type 'help' for more information > > (qemu) device_add macio-oldworld,id=x > > (qemu) device_del x > > (qemu) ** > > ERROR:qom/object.c:1611:object_get_canonical_path_component: > > assertion failed: (obj->parent != NULL) > > Aborted (core dumped) > > > > tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 59 insertions(+), 1 deletion(-) > > > > diff --git a/tests/test-hmp.c b/tests/test-hmp.c > > index 7a38cdc..e8a25c4 100644 > > --- a/tests/test-hmp.c > > +++ b/tests/test-hmp.c > > @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = { > > "commit all", > > "cpu-add 1", > > "cpu 0", > > - "device_add ?", > > "device_add usb-mouse,id=mouse1", > > "mouse_button 7", > > "mouse_move 10 10", > > @@ -116,6 +115,64 @@ static void test_info_commands(void) > > g_free(info_buf); > > } > > > > +/* > > + * People tend to forget to mark internal devices with "user_creatable = false > > + * and these devices can crash QEMU if added via the HMP monitor. So let's run > > + * through all devices and try to add them blindly (without arguments) to see > > + * whether this could crash the QEMU instance. > > + */ > > +static void test_device_add_commands(void) > > +{ > > + char *resp, *devices, *devices_buf, *endp; > > + > > + devices = devices_buf = hmp("device_add help"); > > + > > + while (*devices) { > > + /* Ignore header lines etc. */ > > + if (strncmp(devices, "name \"", 6)) { > > + devices = strchr(devices, '\n'); > > + if (!devices) { > > + break; > > + } > > + devices += 1; > > + continue; > > + } > > + /* Extract the device name, ignore parameters and description */ > > + devices += 6; > > + endp = strchr(devices, '"'); > > + g_assert(endp != NULL); > > + *endp = '\0'; > > + /* Check whether it is blacklisted... */ > > + if (g_str_equal("macio-oldworld", devices)) { > > + devices = strchr(endp + 1, '\n'); > > + if (!devices) { > > + break; > > + } > > + devices += 1; > > + continue; > > + } > > + /* Now run the device_add + device_del commands */ > > + if (verbose) { > > + fprintf(stderr, "\tdevice_add %s,id=%s\n", devices, devices); > > + } > > + resp = hmp("device_add %s,id=%s", devices, devices); > > + g_free(resp); > > + if (verbose) { > > + fprintf(stderr, "\tdevice_del %s\n", devices); > > + } > > + resp = hmp("device_del %s", devices); > > + g_free(resp); > > + /* And move forward to the next line */ > > + devices = strchr(endp + 1, '\n'); > > + if (!devices) { > > + break; > > + } > > + devices += 1; > > + } > > + > > + g_free(devices_buf); > > +} > > + > > static void test_machine(gconstpointer data) > > { > > const char *machine = data; > > @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data) > > qtest_start(args); > > > > test_info_commands(); > > + test_device_add_commands(); > > test_commands(); > > > > qtest_end(); > > This finds devices by parsing output of HMP help. I think you should > use introspection instead, like device-introspect-test does. You may > want to extract common utility code from there. > > The actual device_add and device_del also use HMP. Failures are > ignored. A few device_add failures I'd expect: > > * There is no suitable bus. > > * There are suitable buses, but the default one is full. > > * Mandatory parameters are missing, such as device backend. > > * The bus doesn't support hot plug (some other bus might). > > * The device supports only cold plug with -device, not hot plug with > device_add. > > I'm afraid the test only tests one of these common failure modes for > many devices. > > device_del failures I'd expect: > > * The device doesn't exist, because it hasn't completed hot plug, yet. > In some cases such as ACPI PCI hot plug, hot plug may require guest > cooperation, which may take unbounded time. device_add merely kicks > off the hot plug then. I can't remember how to poll for completion. > I also can't remember why we don't send a QMP event. > > The hot plug usually completes quickly, but it may take its own sweet > time, or not complete at all, say because the guest doesn't support > ACPI, or just doesn't feel like plugging right now. > > The test needs to set up a guest that cooperates. I guess that > involves libqos; yet another thing I've forgotten. > > * Same for device_del. You should wait for the DEVICE_DELETED event > with a suitable timeout. Yes, I think that's an interesting problem - although the test might still be worth it just to see how many issues it finds; I'm curious how many devices actually work with no parameters though, most seem to fail. If I'm reading the code right it's creating the device with the same name as the device; I wonder if that always works? But still, it should mean that if the previous hotplug hasn't really happened then you can move onto the next add. > We could improve device_add to cold plug before the machine starts, > i.e. between start with -S and the first cont. We could similarly > improve device_del to cold plug. Together, that would let you sidestep > the hot plug complications. > > I guess this test is a case of "if it was easy, we would've done it > already"... Still maybe it's worth it as a start. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-05 16:48 ` Dr. David Alan Gilbert @ 2017-09-05 18:11 ` Thomas Huth 2017-09-05 18:37 ` Dr. David Alan Gilbert 2017-09-06 6:59 ` Markus Armbruster 0 siblings, 2 replies; 13+ messages in thread From: Thomas Huth @ 2017-09-05 18:11 UTC (permalink / raw) To: Dr. David Alan Gilbert, Markus Armbruster Cc: qemu-devel, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber, Philippe Mathieu-Daudé, Eduardo Habkost On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > * Markus Armbruster (armbru@redhat.com) wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> People tend to forget to mark internal devices with "user_creatable = false >>> or hotpluggable = false, and these devices can crash QEMU if added via the >>> HMP monitor. So let's add a test to run through all devices and that tries >>> to add them blindly (without arguments) to see whether this could crash the >>> QEMU instance. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> I've marked the patch as RFC since not all of the required device bug >>> fixes have been merged yet (so this patch can not be included yet without >>> breaking "make check"). It's also sad that "macio-oldworld" currently >>> has to be blacklisted - I tried to find a fix for that device, but I was >>> not able to spot the exact problem so far. So help for fixing that device >>> is very very welcome! The crash can be reproduced like this: >>> >>> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none >>> QEMU 2.10.50 monitor - type 'help' for more information >>> (qemu) device_add macio-oldworld,id=x >>> (qemu) device_del x >>> (qemu) ** >>> ERROR:qom/object.c:1611:object_get_canonical_path_component: >>> assertion failed: (obj->parent != NULL) >>> Aborted (core dumped) >>> >>> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 59 insertions(+), 1 deletion(-) >>> >>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c >>> index 7a38cdc..e8a25c4 100644 >>> --- a/tests/test-hmp.c >>> +++ b/tests/test-hmp.c >>> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = { >>> "commit all", >>> "cpu-add 1", >>> "cpu 0", >>> - "device_add ?", >>> "device_add usb-mouse,id=mouse1", >>> "mouse_button 7", >>> "mouse_move 10 10", >>> @@ -116,6 +115,64 @@ static void test_info_commands(void) >>> g_free(info_buf); >>> } >>> >>> +/* >>> + * People tend to forget to mark internal devices with "user_creatable = false >>> + * and these devices can crash QEMU if added via the HMP monitor. So let's run >>> + * through all devices and try to add them blindly (without arguments) to see >>> + * whether this could crash the QEMU instance. >>> + */ >>> +static void test_device_add_commands(void) >>> +{ >>> + char *resp, *devices, *devices_buf, *endp; >>> + >>> + devices = devices_buf = hmp("device_add help"); >>> + >>> + while (*devices) { >>> + /* Ignore header lines etc. */ >>> + if (strncmp(devices, "name \"", 6)) { >>> + devices = strchr(devices, '\n'); >>> + if (!devices) { >>> + break; >>> + } >>> + devices += 1; >>> + continue; >>> + } >>> + /* Extract the device name, ignore parameters and description */ >>> + devices += 6; >>> + endp = strchr(devices, '"'); >>> + g_assert(endp != NULL); >>> + *endp = '\0'; >>> + /* Check whether it is blacklisted... */ >>> + if (g_str_equal("macio-oldworld", devices)) { >>> + devices = strchr(endp + 1, '\n'); >>> + if (!devices) { >>> + break; >>> + } >>> + devices += 1; >>> + continue; >>> + } >>> + /* Now run the device_add + device_del commands */ >>> + if (verbose) { >>> + fprintf(stderr, "\tdevice_add %s,id=%s\n", devices, devices); >>> + } >>> + resp = hmp("device_add %s,id=%s", devices, devices); >>> + g_free(resp); >>> + if (verbose) { >>> + fprintf(stderr, "\tdevice_del %s\n", devices); >>> + } >>> + resp = hmp("device_del %s", devices); >>> + g_free(resp); >>> + /* And move forward to the next line */ >>> + devices = strchr(endp + 1, '\n'); >>> + if (!devices) { >>> + break; >>> + } >>> + devices += 1; >>> + } >>> + >>> + g_free(devices_buf); >>> +} >>> + >>> static void test_machine(gconstpointer data) >>> { >>> const char *machine = data; >>> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data) >>> qtest_start(args); >>> >>> test_info_commands(); >>> + test_device_add_commands(); >>> test_commands(); >>> >>> qtest_end(); >> >> This finds devices by parsing output of HMP help. I think you should >> use introspection instead, like device-introspect-test does. You may >> want to extract common utility code from there. Well, I wrote a HMP test, to simulate what users can do at the HMP prompt. But ok, it's likely to crash QEMU also when using the QMP interface instead ... but then the code should also go into a different .c file, I think. >> The actual device_add and device_del also use HMP. Failures are >> ignored. A few device_add failures I'd expect: >> >> * There is no suitable bus. >> >> * There are suitable buses, but the default one is full. You can partly work around the above two problems by looping a couple of times through the "device_add"s first, before doing the "device_del"s. Then the first iteration adds additional buses which get populated in the second iteration. I can get additional QEMU crashes if I modify my test that way... but currently I lack time for debugging all these crashes, so I don't want to include that in this patch here yet. >> * Mandatory parameters are missing, such as device backend. That's quite hard to handle even with QMP, isn't it? How should a generic test know which parameter have to be populated with which value? >> * The bus doesn't support hot plug (some other bus might). That should not be a problem - at least QEMU should not crash in this situation. >> * The device supports only cold plug with -device, not hot plug with >> device_add. We've got Eduardo's scripts/device-crash-test script for that already, so no need to cover that here. >> I'm afraid the test only tests one of these common failure modes for >> many devices. >> >> device_del failures I'd expect: >> >> * The device doesn't exist, because it hasn't completed hot plug, yet. >> In some cases such as ACPI PCI hot plug, hot plug may require guest >> cooperation, which may take unbounded time. device_add merely kicks >> off the hot plug then. I can't remember how to poll for completion. >> I also can't remember why we don't send a QMP event. >> >> The hot plug usually completes quickly, but it may take its own sweet >> time, or not complete at all, say because the guest doesn't support >> ACPI, or just doesn't feel like plugging right now. >> >> The test needs to set up a guest that cooperates. I guess that >> involves libqos; yet another thing I've forgotten. That was certainly not my scope when I wrote this test. I just want to get rid of these devices that can easily crash QEMU if you just try to add or remove them at the monitor prompt. A more detailed hotplug test should IMHO be done by the individual device tests instead, like it is done in many tests/virtio*.c and tests/usb*.c files already. >> * Same for device_del. You should wait for the DEVICE_DELETED event >> with a suitable timeout. > > Yes, I think that's an interesting problem - although the test > might still be worth it just to see how many issues it finds; I'm > curious how many devices actually work with no parameters though, > most seem to fail. My test already helped to find lots of problems: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=f58f25599b72c7479e6a1 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=469f3da42ef4af347fa78 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=574ee06de9c4fe63c90be https://git.qemu.org/?p=qemu.git;a=commitdiff;h=84ebd3e8c7d4fe955b359 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0d4fa4996fc5ee56ea7d0 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=1f98e55385d11da1dc0de https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8ccccff9dd7ba24c7a788 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0479097859372a760843a https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a808c0865b720e22ca292 https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04618.html https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04164.html https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00306.html https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04622.html https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04635.html https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05390.html ... so does it now sound at least a little bit usable? > If I'm reading the code right it's creating the device with the same > name as the device; I wonder if that always works? Why not? The id is just an arbitrary string, isn't it? > But still, it should mean that if the previous hotplug hasn't really > happened then you can move onto the next add. > >> We could improve device_add to cold plug before the machine starts, >> i.e. between start with -S and the first cont. We could similarly >> improve device_del to cold plug. Together, that would let you sidestep >> the hot plug complications. >> >> I guess this test is a case of "if it was easy, we would've done it >> already"... > > Still maybe it's worth it as a start. Agreed that we should finally move to a smarter, QMP-based test. But until someone wrote that, maybe we could include this as a temporary guard to avoid that problems like the ones above creep in again? Thomas ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-05 18:11 ` [Qemu-devel] [Qemu-ppc] " Thomas Huth @ 2017-09-05 18:37 ` Dr. David Alan Gilbert 2017-09-06 4:53 ` Thomas Huth 2017-09-06 6:59 ` Markus Armbruster 1 sibling, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2017-09-05 18:37 UTC (permalink / raw) To: Thomas Huth Cc: Markus Armbruster, qemu-devel, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber, Philippe Mathieu-Daudé, Eduardo Habkost * Thomas Huth (thuth@redhat.com) wrote: > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > > * Markus Armbruster (armbru@redhat.com) wrote: > >> Thomas Huth <thuth@redhat.com> writes: > >> > >>> People tend to forget to mark internal devices with "user_creatable = false > >>> or hotpluggable = false, and these devices can crash QEMU if added via the > >>> HMP monitor. So let's add a test to run through all devices and that tries > >>> to add them blindly (without arguments) to see whether this could crash the > >>> QEMU instance. > >>> > >>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>> --- > >>> I've marked the patch as RFC since not all of the required device bug > >>> fixes have been merged yet (so this patch can not be included yet without > >>> breaking "make check"). It's also sad that "macio-oldworld" currently > >>> has to be blacklisted - I tried to find a fix for that device, but I was > >>> not able to spot the exact problem so far. So help for fixing that device > >>> is very very welcome! The crash can be reproduced like this: > >>> > >>> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none > >>> QEMU 2.10.50 monitor - type 'help' for more information > >>> (qemu) device_add macio-oldworld,id=x > >>> (qemu) device_del x > >>> (qemu) ** > >>> ERROR:qom/object.c:1611:object_get_canonical_path_component: > >>> assertion failed: (obj->parent != NULL) > >>> Aborted (core dumped) > >>> > >>> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 59 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c > >>> index 7a38cdc..e8a25c4 100644 > >>> --- a/tests/test-hmp.c > >>> +++ b/tests/test-hmp.c > >>> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = { > >>> "commit all", > >>> "cpu-add 1", > >>> "cpu 0", > >>> - "device_add ?", > >>> "device_add usb-mouse,id=mouse1", > >>> "mouse_button 7", > >>> "mouse_move 10 10", > >>> @@ -116,6 +115,64 @@ static void test_info_commands(void) > >>> g_free(info_buf); > >>> } > >>> > >>> +/* > >>> + * People tend to forget to mark internal devices with "user_creatable = false > >>> + * and these devices can crash QEMU if added via the HMP monitor. So let's run > >>> + * through all devices and try to add them blindly (without arguments) to see > >>> + * whether this could crash the QEMU instance. > >>> + */ > >>> +static void test_device_add_commands(void) > >>> +{ > >>> + char *resp, *devices, *devices_buf, *endp; > >>> + > >>> + devices = devices_buf = hmp("device_add help"); > >>> + > >>> + while (*devices) { > >>> + /* Ignore header lines etc. */ > >>> + if (strncmp(devices, "name \"", 6)) { > >>> + devices = strchr(devices, '\n'); > >>> + if (!devices) { > >>> + break; > >>> + } > >>> + devices += 1; > >>> + continue; > >>> + } > >>> + /* Extract the device name, ignore parameters and description */ > >>> + devices += 6; > >>> + endp = strchr(devices, '"'); > >>> + g_assert(endp != NULL); > >>> + *endp = '\0'; > >>> + /* Check whether it is blacklisted... */ > >>> + if (g_str_equal("macio-oldworld", devices)) { > >>> + devices = strchr(endp + 1, '\n'); > >>> + if (!devices) { > >>> + break; > >>> + } > >>> + devices += 1; > >>> + continue; > >>> + } > >>> + /* Now run the device_add + device_del commands */ > >>> + if (verbose) { > >>> + fprintf(stderr, "\tdevice_add %s,id=%s\n", devices, devices); > >>> + } > >>> + resp = hmp("device_add %s,id=%s", devices, devices); > >>> + g_free(resp); > >>> + if (verbose) { > >>> + fprintf(stderr, "\tdevice_del %s\n", devices); > >>> + } > >>> + resp = hmp("device_del %s", devices); > >>> + g_free(resp); > >>> + /* And move forward to the next line */ > >>> + devices = strchr(endp + 1, '\n'); > >>> + if (!devices) { > >>> + break; > >>> + } > >>> + devices += 1; > >>> + } > >>> + > >>> + g_free(devices_buf); > >>> +} > >>> + > >>> static void test_machine(gconstpointer data) > >>> { > >>> const char *machine = data; > >>> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data) > >>> qtest_start(args); > >>> > >>> test_info_commands(); > >>> + test_device_add_commands(); > >>> test_commands(); > >>> > >>> qtest_end(); > >> > >> This finds devices by parsing output of HMP help. I think you should > >> use introspection instead, like device-introspect-test does. You may > >> want to extract common utility code from there. > > Well, I wrote a HMP test, to simulate what users can do at the HMP > prompt. But ok, it's likely to crash QEMU also when using the QMP > interface instead ... but then the code should also go into a different > .c file, I think. > > >> The actual device_add and device_del also use HMP. Failures are > >> ignored. A few device_add failures I'd expect: > >> > >> * There is no suitable bus. > >> > >> * There are suitable buses, but the default one is full. > > You can partly work around the above two problems by looping a couple of > times through the "device_add"s first, before doing the "device_del"s. > Then the first iteration adds additional buses which get populated in > the second iteration. I can get additional QEMU crashes if I modify my > test that way... but currently I lack time for debugging all these > crashes, so I don't want to include that in this patch here yet. > > >> * Mandatory parameters are missing, such as device backend. > > That's quite hard to handle even with QMP, isn't it? How should a > generic test know which parameter have to be populated with which value? > > >> * The bus doesn't support hot plug (some other bus might). > > That should not be a problem - at least QEMU should not crash in this > situation. > > >> * The device supports only cold plug with -device, not hot plug with > >> device_add. > > We've got Eduardo's scripts/device-crash-test script for that already, > so no need to cover that here. > > >> I'm afraid the test only tests one of these common failure modes for > >> many devices. > >> > >> device_del failures I'd expect: > >> > >> * The device doesn't exist, because it hasn't completed hot plug, yet. > >> In some cases such as ACPI PCI hot plug, hot plug may require guest > >> cooperation, which may take unbounded time. device_add merely kicks > >> off the hot plug then. I can't remember how to poll for completion. > >> I also can't remember why we don't send a QMP event. > >> > >> The hot plug usually completes quickly, but it may take its own sweet > >> time, or not complete at all, say because the guest doesn't support > >> ACPI, or just doesn't feel like plugging right now. > >> > >> The test needs to set up a guest that cooperates. I guess that > >> involves libqos; yet another thing I've forgotten. > > That was certainly not my scope when I wrote this test. I just want to > get rid of these devices that can easily crash QEMU if you just try to > add or remove them at the monitor prompt. A more detailed hotplug test > should IMHO be done by the individual device tests instead, like it is > done in many tests/virtio*.c and tests/usb*.c files already. > > >> * Same for device_del. You should wait for the DEVICE_DELETED event > >> with a suitable timeout. > > > > Yes, I think that's an interesting problem - although the test > > might still be worth it just to see how many issues it finds; I'm > > curious how many devices actually work with no parameters though, > > most seem to fail. > > My test already helped to find lots of problems: > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=f58f25599b72c7479e6a1 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=469f3da42ef4af347fa78 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=574ee06de9c4fe63c90be > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=84ebd3e8c7d4fe955b359 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0d4fa4996fc5ee56ea7d0 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=1f98e55385d11da1dc0de > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8ccccff9dd7ba24c7a788 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0479097859372a760843a > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a808c0865b720e22ca292 > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04618.html > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04164.html > https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00306.html > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04622.html > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04635.html > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05390.html > > ... so does it now sound at least a little bit usable? Seems a good haul; and a good justification for including it. > > If I'm reading the code right it's creating the device with the same > > name as the device; I wonder if that always works? > > Why not? The id is just an arbitrary string, isn't it? I didn't know how arbitrary they were allowed to be and I was also worried they might clash with some existing id. As an example, I see there's at least one device (SUNW,fdtwo) with a , in it's name - is that legal for an id ? > > > But still, it should mean that if the previous hotplug hasn't really > > happened then you can move onto the next add. > > > >> We could improve device_add to cold plug before the machine starts, > >> i.e. between start with -S and the first cont. We could similarly > >> improve device_del to cold plug. Together, that would let you sidestep > >> the hot plug complications. > >> > >> I guess this test is a case of "if it was easy, we would've done it > >> already"... > > > > Still maybe it's worth it as a start. > > Agreed that we should finally move to a smarter, QMP-based test. But > until someone wrote that, maybe we could include this as a temporary > guard to avoid that problems like the ones above creep in again? It seems like a good start. Dave > Thomas -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-05 18:37 ` Dr. David Alan Gilbert @ 2017-09-06 4:53 ` Thomas Huth 0 siblings, 0 replies; 13+ messages in thread From: Thomas Huth @ 2017-09-06 4:53 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Markus Armbruster, qemu-devel, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber, Philippe Mathieu-Daudé, Eduardo Habkost On 05.09.2017 20:37, Dr. David Alan Gilbert wrote: > * Thomas Huth (thuth@redhat.com) wrote: >> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: >>> * Markus Armbruster (armbru@redhat.com) wrote: >>>> Thomas Huth <thuth@redhat.com> writes: >>>> >>>>> People tend to forget to mark internal devices with "user_creatable = false >>>>> or hotpluggable = false, and these devices can crash QEMU if added via the >>>>> HMP monitor. So let's add a test to run through all devices and that tries >>>>> to add them blindly (without arguments) to see whether this could crash the >>>>> QEMU instance. >>>>> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- [...] >>> If I'm reading the code right it's creating the device with the same >>> name as the device; I wonder if that always works? >> >> Why not? The id is just an arbitrary string, isn't it? > > I didn't know how arbitrary they were allowed to be and I was > also worried they might clash with some existing id. > As an example, I see there's at least one device (SUNW,fdtwo) with > a , in it's name - is that legal for an id ? Oh, right, I didn't think of comma :-/ ... I'll try to come up with a better solution in the next version of the patch... Thomas ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-05 18:11 ` [Qemu-devel] [Qemu-ppc] " Thomas Huth 2017-09-05 18:37 ` Dr. David Alan Gilbert @ 2017-09-06 6:59 ` Markus Armbruster 2017-09-09 20:41 ` Eduardo Habkost 1 sibling, 1 reply; 13+ messages in thread From: Markus Armbruster @ 2017-09-06 6:59 UTC (permalink / raw) To: Thomas Huth Cc: Dr. David Alan Gilbert, Eduardo Habkost, Philippe Mathieu-Daudé, qemu-devel, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber Thomas Huth <thuth@redhat.com> writes: > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: >> * Markus Armbruster (armbru@redhat.com) wrote: >>> Thomas Huth <thuth@redhat.com> writes: >>> >>>> People tend to forget to mark internal devices with "user_creatable = false >>>> or hotpluggable = false, and these devices can crash QEMU if added via the >>>> HMP monitor. So let's add a test to run through all devices and that tries >>>> to add them blindly (without arguments) to see whether this could crash the >>>> QEMU instance. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> I've marked the patch as RFC since not all of the required device bug >>>> fixes have been merged yet (so this patch can not be included yet without >>>> breaking "make check"). It's also sad that "macio-oldworld" currently >>>> has to be blacklisted - I tried to find a fix for that device, but I was >>>> not able to spot the exact problem so far. So help for fixing that device >>>> is very very welcome! The crash can be reproduced like this: >>>> >>>> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none >>>> QEMU 2.10.50 monitor - type 'help' for more information >>>> (qemu) device_add macio-oldworld,id=x >>>> (qemu) device_del x >>>> (qemu) ** >>>> ERROR:qom/object.c:1611:object_get_canonical_path_component: >>>> assertion failed: (obj->parent != NULL) >>>> Aborted (core dumped) >>>> >>>> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 59 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c >>>> index 7a38cdc..e8a25c4 100644 >>>> --- a/tests/test-hmp.c >>>> +++ b/tests/test-hmp.c >>>> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = { >>>> "commit all", >>>> "cpu-add 1", >>>> "cpu 0", >>>> - "device_add ?", >>>> "device_add usb-mouse,id=mouse1", >>>> "mouse_button 7", >>>> "mouse_move 10 10", >>>> @@ -116,6 +115,64 @@ static void test_info_commands(void) >>>> g_free(info_buf); >>>> } >>>> >>>> +/* >>>> + * People tend to forget to mark internal devices with "user_creatable = false >>>> + * and these devices can crash QEMU if added via the HMP monitor. So let's run >>>> + * through all devices and try to add them blindly (without arguments) to see >>>> + * whether this could crash the QEMU instance. >>>> + */ >>>> +static void test_device_add_commands(void) >>>> +{ >>>> + char *resp, *devices, *devices_buf, *endp; >>>> + >>>> + devices = devices_buf = hmp("device_add help"); >>>> + >>>> + while (*devices) { >>>> + /* Ignore header lines etc. */ >>>> + if (strncmp(devices, "name \"", 6)) { >>>> + devices = strchr(devices, '\n'); >>>> + if (!devices) { >>>> + break; >>>> + } >>>> + devices += 1; >>>> + continue; >>>> + } >>>> + /* Extract the device name, ignore parameters and description */ >>>> + devices += 6; >>>> + endp = strchr(devices, '"'); >>>> + g_assert(endp != NULL); >>>> + *endp = '\0'; >>>> + /* Check whether it is blacklisted... */ >>>> + if (g_str_equal("macio-oldworld", devices)) { >>>> + devices = strchr(endp + 1, '\n'); >>>> + if (!devices) { >>>> + break; >>>> + } >>>> + devices += 1; >>>> + continue; >>>> + } >>>> + /* Now run the device_add + device_del commands */ >>>> + if (verbose) { >>>> + fprintf(stderr, "\tdevice_add %s,id=%s\n", devices, devices); >>>> + } >>>> + resp = hmp("device_add %s,id=%s", devices, devices); >>>> + g_free(resp); >>>> + if (verbose) { >>>> + fprintf(stderr, "\tdevice_del %s\n", devices); >>>> + } >>>> + resp = hmp("device_del %s", devices); >>>> + g_free(resp); >>>> + /* And move forward to the next line */ >>>> + devices = strchr(endp + 1, '\n'); >>>> + if (!devices) { >>>> + break; >>>> + } >>>> + devices += 1; >>>> + } >>>> + >>>> + g_free(devices_buf); >>>> +} >>>> + >>>> static void test_machine(gconstpointer data) >>>> { >>>> const char *machine = data; >>>> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data) >>>> qtest_start(args); >>>> >>>> test_info_commands(); >>>> + test_device_add_commands(); >>>> test_commands(); >>>> >>>> qtest_end(); >>> >>> This finds devices by parsing output of HMP help. I think you should >>> use introspection instead, like device-introspect-test does. You may >>> want to extract common utility code from there. > > Well, I wrote a HMP test, to simulate what users can do at the HMP > prompt. But ok, it's likely to crash QEMU also when using the QMP > interface instead ... but then the code should also go into a different > .c file, I think. HMP device_add is a trivial wrapper around QMP, as is proper: void hmp_device_add(Monitor *mon, const QDict *qdict) { Error *err = NULL; qmp_device_add((QDict *)qdict, NULL, &err); hmp_handle_error(mon, &err); } If anything ever breaks in this wrapper, it won't be specific to HMP device_add. >>> The actual device_add and device_del also use HMP. Failures are >>> ignored. A few device_add failures I'd expect: >>> >>> * There is no suitable bus. >>> >>> * There are suitable buses, but the default one is full. > > You can partly work around the above two problems by looping a couple of > times through the "device_add"s first, before doing the "device_del"s. > Then the first iteration adds additional buses which get populated in > the second iteration. I can get additional QEMU crashes if I modify my > test that way... but currently I lack time for debugging all these > crashes, so I don't want to include that in this patch here yet. Kind of a random walk. Eduardo has been working on "socket introspection", i.e. means to find available "sockets" for devices to plug into. Could be used to for a more directed walk. Eduardo, any thoughts? >>> * Mandatory parameters are missing, such as device backend. > > That's quite hard to handle even with QMP, isn't it? How should a > generic test know which parameter have to be populated with which value? It could perhaps populate sufficiently generic parameters such as common backends. Sadly, device-list-properties is weak, and unless we improve or replace it, this would involve somewhat ugly hardcoding of descriptions. For instance, we'd have to recognize from {"name": "netdev", "description": "ID of a netdev to use as a backend", "type": "str"} that property netdev is a network backend, and very likely mandatory. >>> * The bus doesn't support hot plug (some other bus might). > > That should not be a problem - at least QEMU should not crash in this > situation. Yes, and testing that has some value. It doesn't test the device, though, only the qdev core. I don't mean to suggest your test is useless. I'm merely pointing at some gaping coverage holes :) >>> * The device supports only cold plug with -device, not hot plug with >>> device_add. > > We've got Eduardo's scripts/device-crash-test script for that already, > so no need to cover that here. Point taken. So this test is really about hot plug / unplug. Suggest to clarify the commit message: s/add them blindly/hotplug and unplug them blindly/. >>> I'm afraid the test only tests one of these common failure modes for >>> many devices. >>> >>> device_del failures I'd expect: >>> >>> * The device doesn't exist, because it hasn't completed hot plug, yet. >>> In some cases such as ACPI PCI hot plug, hot plug may require guest >>> cooperation, which may take unbounded time. device_add merely kicks >>> off the hot plug then. I can't remember how to poll for completion. >>> I also can't remember why we don't send a QMP event. >>> >>> The hot plug usually completes quickly, but it may take its own sweet >>> time, or not complete at all, say because the guest doesn't support >>> ACPI, or just doesn't feel like plugging right now. >>> >>> The test needs to set up a guest that cooperates. I guess that >>> involves libqos; yet another thing I've forgotten. > > That was certainly not my scope when I wrote this test. I just want to > get rid of these devices that can easily crash QEMU if you just try to > add or remove them at the monitor prompt. A more detailed hotplug test > should IMHO be done by the individual device tests instead, like it is > done in many tests/virtio*.c and tests/usb*.c files already. I'm not trying to talk you into widening the scope of your test. I am pointing out that your testing of unplug is racy, and may therefore miss failures randomly: if hotplug is slow, device_del won't actually do anything interesting. >>> * Same for device_del. You should wait for the DEVICE_DELETED event >>> with a suitable timeout. >> >> Yes, I think that's an interesting problem - although the test >> might still be worth it just to see how many issues it finds; I'm >> curious how many devices actually work with no parameters though, >> most seem to fail. > > My test already helped to find lots of problems: > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=f58f25599b72c7479e6a1 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=469f3da42ef4af347fa78 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=574ee06de9c4fe63c90be > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=84ebd3e8c7d4fe955b359 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0d4fa4996fc5ee56ea7d0 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=1f98e55385d11da1dc0de > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8ccccff9dd7ba24c7a788 > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0479097859372a760843a > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a808c0865b720e22ca292 > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04618.html > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04164.html > https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00306.html > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04622.html > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04635.html > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05390.html > > ... so does it now sound at least a little bit usable? I don't doubt it is, but its limitations need to be understood and either relaxed or documented. >> If I'm reading the code right it's creating the device with the same >> name as the device; I wonder if that always works? > > Why not? The id is just an arbitrary string, isn't it? Since you're using HMP, you get to quote ',', which occurs in some device names[*]. Enjoy! ;-P Picking IDs that aren't anti-social may be easier. >> But still, it should mean that if the previous hotplug hasn't really >> happened then you can move onto the next add. >> >>> We could improve device_add to cold plug before the machine starts, >>> i.e. between start with -S and the first cont. We could similarly >>> improve device_del to cold plug. Together, that would let you sidestep >>> the hot plug complications. >>> >>> I guess this test is a case of "if it was easy, we would've done it >>> already"... >> >> Still maybe it's worth it as a start. > > Agreed that we should finally move to a smarter, QMP-based test. But > until someone wrote that, maybe we could include this as a temporary > guard to avoid that problems like the ones above creep in again? I think its limitations need to be understood to a useful degree, and either relaxed or documented. [*] Blame IEEE 1275 device trees. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-06 6:59 ` Markus Armbruster @ 2017-09-09 20:41 ` Eduardo Habkost 2017-09-11 6:13 ` Thomas Huth 0 siblings, 1 reply; 13+ messages in thread From: Eduardo Habkost @ 2017-09-09 20:41 UTC (permalink / raw) To: Markus Armbruster Cc: Thomas Huth, Dr. David Alan Gilbert, Philippe Mathieu-Daudé, qemu-devel, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > > > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > >> * Markus Armbruster (armbru@redhat.com) wrote: > >>> Thomas Huth <thuth@redhat.com> writes: > >>> > >>>> People tend to forget to mark internal devices with "user_creatable = false > >>>> or hotpluggable = false, and these devices can crash QEMU if added via the > >>>> HMP monitor. So let's add a test to run through all devices and that tries > >>>> to add them blindly (without arguments) to see whether this could crash the > >>>> QEMU instance. > >>>> > >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>>> --- > >>>> I've marked the patch as RFC since not all of the required device bug > >>>> fixes have been merged yet (so this patch can not be included yet without > >>>> breaking "make check"). It's also sad that "macio-oldworld" currently > >>>> has to be blacklisted - I tried to find a fix for that device, but I was > >>>> not able to spot the exact problem so far. So help for fixing that device > >>>> is very very welcome! The crash can be reproduced like this: > >>>> > >>>> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none > >>>> QEMU 2.10.50 monitor - type 'help' for more information > >>>> (qemu) device_add macio-oldworld,id=x > >>>> (qemu) device_del x > >>>> (qemu) ** > >>>> ERROR:qom/object.c:1611:object_get_canonical_path_component: > >>>> assertion failed: (obj->parent != NULL) > >>>> Aborted (core dumped) > >>>> > >>>> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 59 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c > >>>> index 7a38cdc..e8a25c4 100644 > >>>> --- a/tests/test-hmp.c > >>>> +++ b/tests/test-hmp.c > >>>> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = { > >>>> "commit all", > >>>> "cpu-add 1", > >>>> "cpu 0", > >>>> - "device_add ?", > >>>> "device_add usb-mouse,id=mouse1", > >>>> "mouse_button 7", > >>>> "mouse_move 10 10", > >>>> @@ -116,6 +115,64 @@ static void test_info_commands(void) > >>>> g_free(info_buf); > >>>> } > >>>> > >>>> +/* > >>>> + * People tend to forget to mark internal devices with "user_creatable = false > >>>> + * and these devices can crash QEMU if added via the HMP monitor. So let's run > >>>> + * through all devices and try to add them blindly (without arguments) to see > >>>> + * whether this could crash the QEMU instance. > >>>> + */ > >>>> +static void test_device_add_commands(void) > >>>> +{ > >>>> + char *resp, *devices, *devices_buf, *endp; > >>>> + > >>>> + devices = devices_buf = hmp("device_add help"); > >>>> + > >>>> + while (*devices) { > >>>> + /* Ignore header lines etc. */ > >>>> + if (strncmp(devices, "name \"", 6)) { > >>>> + devices = strchr(devices, '\n'); > >>>> + if (!devices) { > >>>> + break; > >>>> + } > >>>> + devices += 1; > >>>> + continue; > >>>> + } > >>>> + /* Extract the device name, ignore parameters and description */ > >>>> + devices += 6; > >>>> + endp = strchr(devices, '"'); > >>>> + g_assert(endp != NULL); > >>>> + *endp = '\0'; > >>>> + /* Check whether it is blacklisted... */ > >>>> + if (g_str_equal("macio-oldworld", devices)) { > >>>> + devices = strchr(endp + 1, '\n'); > >>>> + if (!devices) { > >>>> + break; > >>>> + } > >>>> + devices += 1; > >>>> + continue; > >>>> + } > >>>> + /* Now run the device_add + device_del commands */ > >>>> + if (verbose) { > >>>> + fprintf(stderr, "\tdevice_add %s,id=%s\n", devices, devices); > >>>> + } > >>>> + resp = hmp("device_add %s,id=%s", devices, devices); > >>>> + g_free(resp); > >>>> + if (verbose) { > >>>> + fprintf(stderr, "\tdevice_del %s\n", devices); > >>>> + } > >>>> + resp = hmp("device_del %s", devices); > >>>> + g_free(resp); > >>>> + /* And move forward to the next line */ > >>>> + devices = strchr(endp + 1, '\n'); > >>>> + if (!devices) { > >>>> + break; > >>>> + } > >>>> + devices += 1; > >>>> + } > >>>> + > >>>> + g_free(devices_buf); > >>>> +} > >>>> + > >>>> static void test_machine(gconstpointer data) > >>>> { > >>>> const char *machine = data; > >>>> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data) > >>>> qtest_start(args); > >>>> > >>>> test_info_commands(); > >>>> + test_device_add_commands(); > >>>> test_commands(); > >>>> > >>>> qtest_end(); > >>> > >>> This finds devices by parsing output of HMP help. I think you should > >>> use introspection instead, like device-introspect-test does. You may > >>> want to extract common utility code from there. > > > > Well, I wrote a HMP test, to simulate what users can do at the HMP > > prompt. But ok, it's likely to crash QEMU also when using the QMP > > interface instead ... but then the code should also go into a different > > .c file, I think. > > HMP device_add is a trivial wrapper around QMP, as is proper: > > void hmp_device_add(Monitor *mon, const QDict *qdict) > { > Error *err = NULL; > > qmp_device_add((QDict *)qdict, NULL, &err); > hmp_handle_error(mon, &err); > } > > If anything ever breaks in this wrapper, it won't be specific to HMP > device_add. > > >>> The actual device_add and device_del also use HMP. Failures are > >>> ignored. A few device_add failures I'd expect: > >>> > >>> * There is no suitable bus. > >>> > >>> * There are suitable buses, but the default one is full. > > > > You can partly work around the above two problems by looping a couple of > > times through the "device_add"s first, before doing the "device_del"s. > > Then the first iteration adds additional buses which get populated in > > the second iteration. I can get additional QEMU crashes if I modify my > > test that way... but currently I lack time for debugging all these > > crashes, so I don't want to include that in this patch here yet. > > Kind of a random walk. > > Eduardo has been working on "socket introspection", i.e. means to find > available "sockets" for devices to plug into. Could be used to for a > more directed walk. Eduardo, any thoughts? My query-device-slots work in progress included a Python script that tried to validate some of the query-device-slots data. It could include one additional check where it tries to plug devices to those slots and see if something breaks. But considering the way the device hotplug code works today, test code that tries random device-types/arguments that are _not_ reported by query-device-slots would be useful too. > > >>> * Mandatory parameters are missing, such as device backend. > > > > That's quite hard to handle even with QMP, isn't it? How should a > > generic test know which parameter have to be populated with which value? > > It could perhaps populate sufficiently generic parameters such as common > backends. > > Sadly, device-list-properties is weak, and unless we improve or replace > it, this would involve somewhat ugly hardcoding of descriptions. For > instance, we'd have to recognize from > > {"name": "netdev", "description": "ID of a netdev to use as a backend", "type": "str"} > > that property netdev is a network backend, and very likely mandatory. > > >>> * The bus doesn't support hot plug (some other bus might). > > > > That should not be a problem - at least QEMU should not crash in this > > situation. > > Yes, and testing that has some value. It doesn't test the device, > though, only the qdev core. > > I don't mean to suggest your test is useless. I'm merely pointing at > some gaping coverage holes :) > > >>> * The device supports only cold plug with -device, not hot plug with > >>> device_add. > > > > We've got Eduardo's scripts/device-crash-test script for that already, > > so no need to cover that here. > > Point taken. So this test is really about hot plug / unplug. Suggest > to clarify the commit message: s/add them blindly/hotplug and unplug > them blindly/. We could extend device-crash-test to test device_add too, as it already has extra code to deal with known crashes and testing multiple machine-types. Also, any additional code we write to ensure we add mandatory arguments or plug only to valid buses would apply to both -device and device_add. I also think Python test code is easier to maintain and extend, but that's just my personal preference. > > >>> I'm afraid the test only tests one of these common failure modes for > >>> many devices. > >>> > >>> device_del failures I'd expect: > >>> > >>> * The device doesn't exist, because it hasn't completed hot plug, yet. > >>> In some cases such as ACPI PCI hot plug, hot plug may require guest > >>> cooperation, which may take unbounded time. device_add merely kicks > >>> off the hot plug then. I can't remember how to poll for completion. > >>> I also can't remember why we don't send a QMP event. > >>> > >>> The hot plug usually completes quickly, but it may take its own sweet > >>> time, or not complete at all, say because the guest doesn't support > >>> ACPI, or just doesn't feel like plugging right now. > >>> > >>> The test needs to set up a guest that cooperates. I guess that > >>> involves libqos; yet another thing I've forgotten. > > > > That was certainly not my scope when I wrote this test. I just want to > > get rid of these devices that can easily crash QEMU if you just try to > > add or remove them at the monitor prompt. A more detailed hotplug test > > should IMHO be done by the individual device tests instead, like it is > > done in many tests/virtio*.c and tests/usb*.c files already. > > I'm not trying to talk you into widening the scope of your test. I am > pointing out that your testing of unplug is racy, and may therefore miss > failures randomly: if hotplug is slow, device_del won't actually do > anything interesting. > > >>> * Same for device_del. You should wait for the DEVICE_DELETED event > >>> with a suitable timeout. > >> > >> Yes, I think that's an interesting problem - although the test > >> might still be worth it just to see how many issues it finds; I'm > >> curious how many devices actually work with no parameters though, > >> most seem to fail. > > > > My test already helped to find lots of problems: > > > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=f58f25599b72c7479e6a1 > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=469f3da42ef4af347fa78 > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=574ee06de9c4fe63c90be > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=84ebd3e8c7d4fe955b359 > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0d4fa4996fc5ee56ea7d0 > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=1f98e55385d11da1dc0de > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8ccccff9dd7ba24c7a788 > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0479097859372a760843a > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a808c0865b720e22ca292 > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04618.html > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04164.html > > https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00306.html > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04622.html > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04635.html > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05390.html > > > > ... so does it now sound at least a little bit usable? > > I don't doubt it is, but its limitations need to be understood and > either relaxed or documented. > > >> If I'm reading the code right it's creating the device with the same > >> name as the device; I wonder if that always works? > > > > Why not? The id is just an arbitrary string, isn't it? > > Since you're using HMP, you get to quote ',', which occurs in some > device names[*]. Enjoy! ;-P > > Picking IDs that aren't anti-social may be easier. > > >> But still, it should mean that if the previous hotplug hasn't really > >> happened then you can move onto the next add. > >> > >>> We could improve device_add to cold plug before the machine starts, > >>> i.e. between start with -S and the first cont. We could similarly > >>> improve device_del to cold plug. Together, that would let you sidestep > >>> the hot plug complications. > >>> > >>> I guess this test is a case of "if it was easy, we would've done it > >>> already"... > >> > >> Still maybe it's worth it as a start. > > > > Agreed that we should finally move to a smarter, QMP-based test. But > > until someone wrote that, maybe we could include this as a temporary > > guard to avoid that problems like the ones above creep in again? > > I think its limitations need to be understood to a useful degree, and > either relaxed or documented. > > > [*] Blame IEEE 1275 device trees. -- Eduardo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-09 20:41 ` Eduardo Habkost @ 2017-09-11 6:13 ` Thomas Huth 2017-09-12 17:37 ` Eduardo Habkost 0 siblings, 1 reply; 13+ messages in thread From: Thomas Huth @ 2017-09-11 6:13 UTC (permalink / raw) To: Eduardo Habkost, Markus Armbruster Cc: qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber, Philippe Mathieu-Daudé On 09.09.2017 22:41, Eduardo Habkost wrote: > On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: >>>> * Markus Armbruster (armbru@redhat.com) wrote: >>>>> Thomas Huth <thuth@redhat.com> writes: >>>>> >>>>>> People tend to forget to mark internal devices with "user_creatable = false >>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the >>>>>> HMP monitor. So let's add a test to run through all devices and that tries >>>>>> to add them blindly (without arguments) to see whether this could crash the >>>>>> QEMU instance. [...] >>>>> * The device supports only cold plug with -device, not hot plug with >>>>> device_add. >>> >>> We've got Eduardo's scripts/device-crash-test script for that already, >>> so no need to cover that here. >> >> Point taken. So this test is really about hot plug / unplug. Suggest >> to clarify the commit message: s/add them blindly/hotplug and unplug >> them blindly/. > > We could extend device-crash-test to test device_add too, as it > already has extra code to deal with known crashes and testing > multiple machine-types. Also, any additional code we write to > ensure we add mandatory arguments or plug only to valid buses > would apply to both -device and device_add. I also think Python > test code is easier to maintain and extend, but that's just my > personal preference. Adding device_add/del support to device-crash-test is certainly an option. The problem is that nobody runs it by default, so this won't help to avoid that new problems are being committed to the repository. I think we really should have a test for "make check", too. So would my test be acceptable if I'd rewrite it to use QMP instead (I don't think I could do the full list that Markus mentioned, but at least a basic test via QMP as a start)? >>>> If I'm reading the code right it's creating the device with the same >>>> name as the device; I wonder if that always works? >>> >>> Why not? The id is just an arbitrary string, isn't it? >> >> Since you're using HMP, you get to quote ',', which occurs in some >> device names[*]. Enjoy! ;-P >> >> Picking IDs that aren't anti-social may be easier. I'm considering to fail the test if it detects a device with a ',' in its name. Such devices should really not be there in QEMU... Thomas ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-11 6:13 ` Thomas Huth @ 2017-09-12 17:37 ` Eduardo Habkost 2017-09-13 5:45 ` Thomas Huth 0 siblings, 1 reply; 13+ messages in thread From: Eduardo Habkost @ 2017-09-12 17:37 UTC (permalink / raw) To: Thomas Huth Cc: Markus Armbruster, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber, Philippe Mathieu-Daudé On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote: > On 09.09.2017 22:41, Eduardo Habkost wrote: > > On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote: > >> Thomas Huth <thuth@redhat.com> writes: > >> > >>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > >>>> * Markus Armbruster (armbru@redhat.com) wrote: > >>>>> Thomas Huth <thuth@redhat.com> writes: > >>>>> > >>>>>> People tend to forget to mark internal devices with "user_creatable = false > >>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the > >>>>>> HMP monitor. So let's add a test to run through all devices and that tries > >>>>>> to add them blindly (without arguments) to see whether this could crash the > >>>>>> QEMU instance. > [...] > >>>>> * The device supports only cold plug with -device, not hot plug with > >>>>> device_add. > >>> > >>> We've got Eduardo's scripts/device-crash-test script for that already, > >>> so no need to cover that here. > >> > >> Point taken. So this test is really about hot plug / unplug. Suggest > >> to clarify the commit message: s/add them blindly/hotplug and unplug > >> them blindly/. > > > > We could extend device-crash-test to test device_add too, as it > > already has extra code to deal with known crashes and testing > > multiple machine-types. Also, any additional code we write to > > ensure we add mandatory arguments or plug only to valid buses > > would apply to both -device and device_add. I also think Python > > test code is easier to maintain and extend, but that's just my > > personal preference. > > Adding device_add/del support to device-crash-test is certainly an > option. The problem is that nobody runs it by default, so this won't > help to avoid that new problems are being committed to the repository. > > I think we really should have a test for "make check", too. So would my > test be acceptable if I'd rewrite it to use QMP instead (I don't think I > could do the full list that Markus mentioned, but at least a basic test > via QMP as a start)? We can run device-crash-test on "make check", we just need to choose what's the subset of tests we want to run (because testing all machine+device+target combinations would take too long). But while device-crash-test doesn't support hotplug, I think your test code would be good too. -- Eduardo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-12 17:37 ` Eduardo Habkost @ 2017-09-13 5:45 ` Thomas Huth 2017-09-15 22:18 ` Eduardo Habkost 0 siblings, 1 reply; 13+ messages in thread From: Thomas Huth @ 2017-09-13 5:45 UTC (permalink / raw) To: Eduardo Habkost Cc: Markus Armbruster, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber, Philippe Mathieu-Daudé On 12.09.2017 19:37, Eduardo Habkost wrote: > On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote: >> On 09.09.2017 22:41, Eduardo Habkost wrote: >>> On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote: >>>> Thomas Huth <thuth@redhat.com> writes: >>>> >>>>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: >>>>>> * Markus Armbruster (armbru@redhat.com) wrote: >>>>>>> Thomas Huth <thuth@redhat.com> writes: >>>>>>> >>>>>>>> People tend to forget to mark internal devices with "user_creatable = false >>>>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the >>>>>>>> HMP monitor. So let's add a test to run through all devices and that tries >>>>>>>> to add them blindly (without arguments) to see whether this could crash the >>>>>>>> QEMU instance. >> [...] >>>>>>> * The device supports only cold plug with -device, not hot plug with >>>>>>> device_add. >>>>> >>>>> We've got Eduardo's scripts/device-crash-test script for that already, >>>>> so no need to cover that here. >>>> >>>> Point taken. So this test is really about hot plug / unplug. Suggest >>>> to clarify the commit message: s/add them blindly/hotplug and unplug >>>> them blindly/. >>> >>> We could extend device-crash-test to test device_add too, as it >>> already has extra code to deal with known crashes and testing >>> multiple machine-types. Also, any additional code we write to >>> ensure we add mandatory arguments or plug only to valid buses >>> would apply to both -device and device_add. I also think Python >>> test code is easier to maintain and extend, but that's just my >>> personal preference. >> >> Adding device_add/del support to device-crash-test is certainly an >> option. The problem is that nobody runs it by default, so this won't >> help to avoid that new problems are being committed to the repository. >> >> I think we really should have a test for "make check", too. So would my >> test be acceptable if I'd rewrite it to use QMP instead (I don't think I >> could do the full list that Markus mentioned, but at least a basic test >> via QMP as a start)? > > We can run device-crash-test on "make check", we just need to > choose what's the subset of tests we want to run (because testing > all machine+device+target combinations would take too long). Maybe we should just run it one time for every machine - and try to add all available devices at once? Thomas ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-13 5:45 ` Thomas Huth @ 2017-09-15 22:18 ` Eduardo Habkost 2017-09-19 5:25 ` Thomas Huth 0 siblings, 1 reply; 13+ messages in thread From: Eduardo Habkost @ 2017-09-15 22:18 UTC (permalink / raw) To: Thomas Huth Cc: Markus Armbruster, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber, Philippe Mathieu-Daudé On Wed, Sep 13, 2017 at 07:45:11AM +0200, Thomas Huth wrote: > On 12.09.2017 19:37, Eduardo Habkost wrote: > > On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote: > >> On 09.09.2017 22:41, Eduardo Habkost wrote: > >>> On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote: > >>>> Thomas Huth <thuth@redhat.com> writes: > >>>> > >>>>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > >>>>>> * Markus Armbruster (armbru@redhat.com) wrote: > >>>>>>> Thomas Huth <thuth@redhat.com> writes: > >>>>>>> > >>>>>>>> People tend to forget to mark internal devices with "user_creatable = false > >>>>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the > >>>>>>>> HMP monitor. So let's add a test to run through all devices and that tries > >>>>>>>> to add them blindly (without arguments) to see whether this could crash the > >>>>>>>> QEMU instance. > >> [...] > >>>>>>> * The device supports only cold plug with -device, not hot plug with > >>>>>>> device_add. > >>>>> > >>>>> We've got Eduardo's scripts/device-crash-test script for that already, > >>>>> so no need to cover that here. > >>>> > >>>> Point taken. So this test is really about hot plug / unplug. Suggest > >>>> to clarify the commit message: s/add them blindly/hotplug and unplug > >>>> them blindly/. > >>> > >>> We could extend device-crash-test to test device_add too, as it > >>> already has extra code to deal with known crashes and testing > >>> multiple machine-types. Also, any additional code we write to > >>> ensure we add mandatory arguments or plug only to valid buses > >>> would apply to both -device and device_add. I also think Python > >>> test code is easier to maintain and extend, but that's just my > >>> personal preference. > >> > >> Adding device_add/del support to device-crash-test is certainly an > >> option. The problem is that nobody runs it by default, so this won't > >> help to avoid that new problems are being committed to the repository. > >> > >> I think we really should have a test for "make check", too. So would my > >> test be acceptable if I'd rewrite it to use QMP instead (I don't think I > >> could do the full list that Markus mentioned, but at least a basic test > >> via QMP as a start)? > > > > We can run device-crash-test on "make check", we just need to > > choose what's the subset of tests we want to run (because testing > > all machine+device+target combinations would take too long). > > Maybe we should just run it one time for every machine - and try to add > all available devices at once? Yes, it makes sense. I will keep that in mind when trying to implement device_add support on device-crash-test (but if anybody wants to volunteer to implement it, be my guest). -- Eduardo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test 2017-09-15 22:18 ` Eduardo Habkost @ 2017-09-19 5:25 ` Thomas Huth 0 siblings, 0 replies; 13+ messages in thread From: Thomas Huth @ 2017-09-19 5:25 UTC (permalink / raw) To: Eduardo Habkost Cc: Markus Armbruster, qemu-devel, Dr. David Alan Gilbert, qemu-ppc, Igor Mammedov, John Snow, Andreas Färber, Philippe Mathieu-Daudé On 16.09.2017 00:18, Eduardo Habkost wrote: > On Wed, Sep 13, 2017 at 07:45:11AM +0200, Thomas Huth wrote: >> On 12.09.2017 19:37, Eduardo Habkost wrote: >>> On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote: >>>> On 09.09.2017 22:41, Eduardo Habkost wrote: >>>>> On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote: >>>>>> Thomas Huth <thuth@redhat.com> writes: >>>>>> >>>>>>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: >>>>>>>> * Markus Armbruster (armbru@redhat.com) wrote: >>>>>>>>> Thomas Huth <thuth@redhat.com> writes: >>>>>>>>> >>>>>>>>>> People tend to forget to mark internal devices with "user_creatable = false >>>>>>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the >>>>>>>>>> HMP monitor. So let's add a test to run through all devices and that tries >>>>>>>>>> to add them blindly (without arguments) to see whether this could crash the >>>>>>>>>> QEMU instance. >>>> [...] >>>>>>>>> * The device supports only cold plug with -device, not hot plug with >>>>>>>>> device_add. >>>>>>> >>>>>>> We've got Eduardo's scripts/device-crash-test script for that already, >>>>>>> so no need to cover that here. >>>>>> >>>>>> Point taken. So this test is really about hot plug / unplug. Suggest >>>>>> to clarify the commit message: s/add them blindly/hotplug and unplug >>>>>> them blindly/. >>>>> >>>>> We could extend device-crash-test to test device_add too, as it >>>>> already has extra code to deal with known crashes and testing >>>>> multiple machine-types. Also, any additional code we write to >>>>> ensure we add mandatory arguments or plug only to valid buses >>>>> would apply to both -device and device_add. I also think Python >>>>> test code is easier to maintain and extend, but that's just my >>>>> personal preference. >>>> >>>> Adding device_add/del support to device-crash-test is certainly an >>>> option. The problem is that nobody runs it by default, so this won't >>>> help to avoid that new problems are being committed to the repository. >>>> >>>> I think we really should have a test for "make check", too. So would my >>>> test be acceptable if I'd rewrite it to use QMP instead (I don't think I >>>> could do the full list that Markus mentioned, but at least a basic test >>>> via QMP as a start)? >>> >>> We can run device-crash-test on "make check", we just need to >>> choose what's the subset of tests we want to run (because testing >>> all machine+device+target combinations would take too long). >> >> Maybe we should just run it one time for every machine - and try to add >> all available devices at once? > > Yes, it makes sense. I will keep that in mind when trying to > implement device_add support on device-crash-test (but if anybody > wants to volunteer to implement it, be my guest). Never mind, that was a unrealistic idea, since there are very likely devices in the list that prevent QEMU from starting if a certain property is missing... so we likely won't catch any crashes with such a test (unless we want to pedanticly maintain a blacklist of devices which can not be used in that test ... you certainly got a very good start for that in device-crash-test already, but I think the list will rather explode if we want to get it usable for this idea?) Thomas ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-09-19 5:26 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-05 9:53 [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test Thomas Huth 2017-09-05 11:42 ` Markus Armbruster 2017-09-05 16:48 ` Dr. David Alan Gilbert 2017-09-05 18:11 ` [Qemu-devel] [Qemu-ppc] " Thomas Huth 2017-09-05 18:37 ` Dr. David Alan Gilbert 2017-09-06 4:53 ` Thomas Huth 2017-09-06 6:59 ` Markus Armbruster 2017-09-09 20:41 ` Eduardo Habkost 2017-09-11 6:13 ` Thomas Huth 2017-09-12 17:37 ` Eduardo Habkost 2017-09-13 5:45 ` Thomas Huth 2017-09-15 22:18 ` Eduardo Habkost 2017-09-19 5:25 ` Thomas Huth
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.