Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
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 Huthwrites: >> >>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: * Markus Armbruster (arm...@redhat.com) wrote: > Thomas Huth 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
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 Huthwrites: > > > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > >> * Markus Armbruster (arm...@redhat.com) wrote: > >>> Thomas Huth 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
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 Huthwrites: > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: >> * Markus Armbruster (arm...@redhat.com) wrote: >>> Thomas Huth 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
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 Huthwrites: > >> > >>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > * Markus Armbruster (arm...@redhat.com) wrote: > > Thomas Huth 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
On 09.09.2017 22:41, Eduardo Habkost wrote: > On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote: >> Thomas Huthwrites: >> >>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: * Markus Armbruster (arm...@redhat.com) wrote: > Thomas Huth 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote: > Thomas Huthwrites: > > > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > >> * Markus Armbruster (arm...@redhat.com) wrote: > >>> Thomas Huth 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 > --- > 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
Thomas Huthwrites: > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: >> * Markus Armbruster (arm...@redhat.com) wrote: >>> Thomas Huth 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 --- 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
On 05.09.2017 20:37, Dr. David Alan Gilbert wrote: > * Thomas Huth (th...@redhat.com) wrote: >> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: >>> * Markus Armbruster (arm...@redhat.com) wrote: Thomas Huthwrites: > 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 > --- [...] >>> 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
* Thomas Huth (th...@redhat.com) wrote: > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > > * Markus Armbruster (arm...@redhat.com) wrote: > >> Thomas Huthwrites: > >> > >>> 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 > >>> --- > >>> 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
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test
On 05.09.2017 18:48, Dr. David Alan Gilbert wrote: > * Markus Armbruster (arm...@redhat.com) wrote: >> Thomas Huthwrites: >> >>> 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 >>> --- >>> 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