[{"id":1763257,"web_url":"http://patchwork.ozlabs.org/comment/1763257/","msgid":"<871snlwev0.fsf@dusky.pond.sub.org>","list_archive_url":null,"date":"2017-09-05T11:42:59","subject":"Re: [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test","submitter":{"id":2645,"url":"http://patchwork.ozlabs.org/api/people/2645/","name":"Markus Armbruster","email":"armbru@redhat.com"},"content":"Thomas Huth writes:\n\n> People tend to forget to mark internal devices with \"user_creatable = false\n> or hotpluggable = false, and these devices can crash QEMU if added via the\n> HMP monitor. So let's add a test to run through all devices and that tries\n> to add them blindly (without arguments) to see whether this could crash the\n> QEMU instance.\n>\n> Signed-off-by: Thomas Huth \n> ---\n> I've marked the patch as RFC since not all of the required device bug\n> fixes have been merged yet (so this patch can not be included yet without\n> breaking \"make check\"). It's also sad that \"macio-oldworld\" currently\n> has to be blacklisted - I tried to find a fix for that device, but I was\n> not able to spot the exact problem so far. So help for fixing that device\n> is very very welcome! The crash can be reproduced like this:\n>\n> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none\n> QEMU 2.10.50 monitor - type 'help' for more information\n> (qemu) device_add macio-oldworld,id=x\n> (qemu) device_del x\n> (qemu) **\n> ERROR:qom/object.c:1611:object_get_canonical_path_component:\n> assertion failed: (obj->parent != NULL)\n> Aborted (core dumped)\n>\n> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 59 insertions(+), 1 deletion(-)\n>\n> diff --git a/tests/test-hmp.c b/tests/test-hmp.c\n> index 7a38cdc..e8a25c4 100644\n> --- a/tests/test-hmp.c\n> +++ b/tests/test-hmp.c\n> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = {\n> \"commit all\",\n> \"cpu-add 1\",\n> \"cpu 0\",\n> - \"device_add ?\",\n> \"device_add usb-mouse,id=mouse1\",\n> \"mouse_button 7\",\n> \"mouse_move 10 10\",\n> @@ -116,6 +115,64 @@ static void test_info_commands(void)\n> g_free(info_buf);\n> }\n> \n> +/*\n> + * People tend to forget to mark internal devices with \"user_creatable = false\n> + * and these devices can crash QEMU if added via the HMP monitor. So let's run\n> + * through all devices and try to add them blindly (without arguments) to see\n> + * whether this could crash the QEMU instance.\n> + */\n> +static void test_device_add_commands(void)\n> +{\n> + char *resp, *devices, *devices_buf, *endp;\n> +\n> + devices = devices_buf = hmp(\"device_add help\");\n> +\n> + while (*devices) {\n> + /* Ignore header lines etc. */\n> + if (strncmp(devices, \"name \\\"\", 6)) {\n> + devices = strchr(devices, '\\n');\n> + if (!devices) {\n> + break;\n> + }\n> + devices += 1;\n> + continue;\n> + }\n> + /* Extract the device name, ignore parameters and description */\n> + devices += 6;\n> + endp = strchr(devices, '\"');\n> + g_assert(endp != NULL);\n> + *endp = '\\0';\n> + /* Check whether it is blacklisted... */\n> + if (g_str_equal(\"macio-oldworld\", devices)) {\n> + devices = strchr(endp + 1, '\\n');\n> + if (!devices) {\n> + break;\n> + }\n> + devices += 1;\n> + continue;\n> + }\n> + /* Now run the device_add + device_del commands */\n> + if (verbose) {\n> + fprintf(stderr, \"\\tdevice_add %s,id=%s\\n\", devices, devices);\n> + }\n> + resp = hmp(\"device_add %s,id=%s\", devices, devices);\n> + g_free(resp);\n> + if (verbose) {\n> + fprintf(stderr, \"\\tdevice_del %s\\n\", devices);\n> + }\n> + resp = hmp(\"device_del %s\", devices);\n> + g_free(resp);\n> + /* And move forward to the next line */\n> + devices = strchr(endp + 1, '\\n');\n> + if (!devices) {\n> + break;\n> + }\n> + devices += 1;\n> + }\n> +\n> + g_free(devices_buf);\n> +}\n> +\n> static void test_machine(gconstpointer data)\n> {\n> const char *machine = data;\n> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data)\n> qtest_start(args);\n> \n> test_info_commands();\n> + test_device_add_commands();\n> test_commands();\n> \n> qtest_end();\n\nThis finds devices by parsing output of HMP help. I think you should\nuse introspection instead, like device-introspect-test does. You may\nwant to extract common utility code from there.\n\nThe actual device_add and device_del also use HMP. Failures are\nignored. A few device_add failures I'd expect:\n\n* There is no suitable bus.\n\n* There are suitable buses, but the default one is full.\n\n* Mandatory parameters are missing, such as device backend.\n\n* The bus doesn't support hot plug (some other bus might).\n\n* The device supports only cold plug with -device, not hot plug with\n device_add.\n\nI'm afraid the test only tests one of these common failure modes for\nmany devices.\n\ndevice_del failures I'd expect:\n\n* The device doesn't exist, because it hasn't completed hot plug, yet.\n In some cases such as ACPI PCI hot plug, hot plug may require guest\n cooperation, which may take unbounded time. device_add merely kicks\n off the hot plug then. I can't remember how to poll for completion.\n I also can't remember why we don't send a QMP event.\n\n The hot plug usually completes quickly, but it may take its own sweet\n time, or not complete at all, say because the guest doesn't support\n ACPI, or just doesn't feel like plugging right now.\n\n The test needs to set up a guest that cooperates. I guess that\n involves libqos; yet another thing I've forgotten.\n\n* Same for device_del. You should wait for the DEVICE_DELETED event\n with a suitable timeout.\n\nWe could improve device_add to cold plug before the machine starts,\ni.e. between start with -S and the first cont. We could similarly\nimprove device_del to cold plug. Together, that would let you sidestep\nthe hot plug complications.\n\nI guess this test is a case of \"if it was easy, we would've done it\nalready\"...","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=armbru@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xmlGs5bZFz9sNq\n\tfor ;\n\tTue, 5 Sep 2017 21:43:48 +1000 (AEST)","from localhost ([::1]:58230 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpCGe-0005ML-VZ\n\tfor incoming@patchwork.ozlabs.org; Tue, 05 Sep 2017 07:43:45 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56936)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpCG8-0005Lp-A7\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 07:43:20 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpCG3-0007yp-5Q\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 07:43:12 -0400","from mx1.redhat.com ([209.132.183.28]:60476)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpCG2-0007y8-SV; Tue, 05 Sep 2017 07:43:07 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id C18A461D3F;\n\tTue, 5 Sep 2017 11:43:05 +0000 (UTC)","from blackfin.pond.sub.org (ovpn-116-75.ams2.redhat.com\n\t[10.36.116.75])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 4049A77D43;\n\tTue, 5 Sep 2017 11:43:01 +0000 (UTC)","by blackfin.pond.sub.org (Postfix, from userid 1000)\n\tid 9D9BB1138645; Tue, 5 Sep 2017 13:42:59 +0200 (CEST)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com C18A461D3F","From":"Markus Armbruster ","To":"Thomas Huth ","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>","Date":"Tue, 05 Sep 2017 13:42:59 +0200","In-Reply-To":"<1504605227-5124-1-git-send-email-thuth@redhat.com> (Thomas\n\tHuth's message of \"Tue, 5 Sep 2017 11:53:47 +0200\")","Message-ID":"<871snlwev0.fsf@dusky.pond.sub.org>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tTue, 05 Sep 2017 11:43:05 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"qemu-devel@nongnu.org, Alexander Graf , Philippe\n\t=?utf-8?q?Mathieu-Daud=C3=A9?= , qemu-ppc@nongnu.org,\n\tIgor Mammedov , John Snow ,\n\tAndreas =?utf-8?q?F=C3=A4rber?= ,\n\t\"Dr. David Alan Gilbert\" ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1763504,"web_url":"http://patchwork.ozlabs.org/comment/1763504/","msgid":"<20170905164835.GF2112@work-vm>","list_archive_url":null,"date":"2017-09-05T16:48:36","subject":"Re: [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test","submitter":{"id":48102,"url":"http://patchwork.ozlabs.org/api/people/48102/","name":"Dr. David Alan Gilbert","email":"dgilbert@redhat.com"},"content":"* Markus Armbruster (armbru@redhat.com) wrote:\n> Thomas Huth writes:\n> \n> > People tend to forget to mark internal devices with \"user_creatable = false\n> > or hotpluggable = false, and these devices can crash QEMU if added via the\n> > HMP monitor. So let's add a test to run through all devices and that tries\n> > to add them blindly (without arguments) to see whether this could crash the\n> > QEMU instance.\n> >\n> > Signed-off-by: Thomas Huth \n> > ---\n> > I've marked the patch as RFC since not all of the required device bug\n> > fixes have been merged yet (so this patch can not be included yet without\n> > breaking \"make check\"). It's also sad that \"macio-oldworld\" currently\n> > has to be blacklisted - I tried to find a fix for that device, but I was\n> > not able to spot the exact problem so far. So help for fixing that device\n> > is very very welcome! The crash can be reproduced like this:\n> >\n> > $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none\n> > QEMU 2.10.50 monitor - type 'help' for more information\n> > (qemu) device_add macio-oldworld,id=x\n> > (qemu) device_del x\n> > (qemu) **\n> > ERROR:qom/object.c:1611:object_get_canonical_path_component:\n> > assertion failed: (obj->parent != NULL)\n> > Aborted (core dumped)\n> >\n> > tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> > 1 file changed, 59 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/tests/test-hmp.c b/tests/test-hmp.c\n> > index 7a38cdc..e8a25c4 100644\n> > --- a/tests/test-hmp.c\n> > +++ b/tests/test-hmp.c\n> > @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = {\n> > \"commit all\",\n> > \"cpu-add 1\",\n> > \"cpu 0\",\n> > - \"device_add ?\",\n> > \"device_add usb-mouse,id=mouse1\",\n> > \"mouse_button 7\",\n> > \"mouse_move 10 10\",\n> > @@ -116,6 +115,64 @@ static void test_info_commands(void)\n> > g_free(info_buf);\n> > }\n> > \n> > +/*\n> > + * People tend to forget to mark internal devices with \"user_creatable = false\n> > + * and these devices can crash QEMU if added via the HMP monitor. So let's run\n> > + * through all devices and try to add them blindly (without arguments) to see\n> > + * whether this could crash the QEMU instance.\n> > + */\n> > +static void test_device_add_commands(void)\n> > +{\n> > + char *resp, *devices, *devices_buf, *endp;\n> > +\n> > + devices = devices_buf = hmp(\"device_add help\");\n> > +\n> > + while (*devices) {\n> > + /* Ignore header lines etc. */\n> > + if (strncmp(devices, \"name \\\"\", 6)) {\n> > + devices = strchr(devices, '\\n');\n> > + if (!devices) {\n> > + break;\n> > + }\n> > + devices += 1;\n> > + continue;\n> > + }\n> > + /* Extract the device name, ignore parameters and description */\n> > + devices += 6;\n> > + endp = strchr(devices, '\"');\n> > + g_assert(endp != NULL);\n> > + *endp = '\\0';\n> > + /* Check whether it is blacklisted... */\n> > + if (g_str_equal(\"macio-oldworld\", devices)) {\n> > + devices = strchr(endp + 1, '\\n');\n> > + if (!devices) {\n> > + break;\n> > + }\n> > + devices += 1;\n> > + continue;\n> > + }\n> > + /* Now run the device_add + device_del commands */\n> > + if (verbose) {\n> > + fprintf(stderr, \"\\tdevice_add %s,id=%s\\n\", devices, devices);\n> > + }\n> > + resp = hmp(\"device_add %s,id=%s\", devices, devices);\n> > + g_free(resp);\n> > + if (verbose) {\n> > + fprintf(stderr, \"\\tdevice_del %s\\n\", devices);\n> > + }\n> > + resp = hmp(\"device_del %s\", devices);\n> > + g_free(resp);\n> > + /* And move forward to the next line */\n> > + devices = strchr(endp + 1, '\\n');\n> > + if (!devices) {\n> > + break;\n> > + }\n> > + devices += 1;\n> > + }\n> > +\n> > + g_free(devices_buf);\n> > +}\n> > +\n> > static void test_machine(gconstpointer data)\n> > {\n> > const char *machine = data;\n> > @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data)\n> > qtest_start(args);\n> > \n> > test_info_commands();\n> > + test_device_add_commands();\n> > test_commands();\n> > \n> > qtest_end();\n> \n> This finds devices by parsing output of HMP help. I think you should\n> use introspection instead, like device-introspect-test does. You may\n> want to extract common utility code from there.\n> \n> The actual device_add and device_del also use HMP. Failures are\n> ignored. A few device_add failures I'd expect:\n> \n> * There is no suitable bus.\n> \n> * There are suitable buses, but the default one is full.\n> \n> * Mandatory parameters are missing, such as device backend.\n> \n> * The bus doesn't support hot plug (some other bus might).\n> \n> * The device supports only cold plug with -device, not hot plug with\n> device_add.\n> \n> I'm afraid the test only tests one of these common failure modes for\n> many devices.\n> \n> device_del failures I'd expect:\n> \n> * The device doesn't exist, because it hasn't completed hot plug, yet.\n> In some cases such as ACPI PCI hot plug, hot plug may require guest\n> cooperation, which may take unbounded time. device_add merely kicks\n> off the hot plug then. I can't remember how to poll for completion.\n> I also can't remember why we don't send a QMP event.\n> \n> The hot plug usually completes quickly, but it may take its own sweet\n> time, or not complete at all, say because the guest doesn't support\n> ACPI, or just doesn't feel like plugging right now.\n> \n> The test needs to set up a guest that cooperates. I guess that\n> involves libqos; yet another thing I've forgotten.\n> \n> * Same for device_del. You should wait for the DEVICE_DELETED event\n> with a suitable timeout.\n\nYes, I think that's an interesting problem - although the test\nmight still be worth it just to see how many issues it finds; I'm\ncurious how many devices actually work with no parameters though,\nmost seem to fail.\n\nIf I'm reading the code right it's creating the device with the same\nname as the device; I wonder if that always works?\nBut still, it should mean that if the previous hotplug hasn't really\nhappened then you can move onto the next add.\n\n> We could improve device_add to cold plug before the machine starts,\n> i.e. between start with -S and the first cont. We could similarly\n> improve device_del to cold plug. Together, that would let you sidestep\n> the hot plug complications.\n> \n> I guess this test is a case of \"if it was easy, we would've done it\n> already\"...\n\nStill maybe it's worth it as a start.\n\nDave\n\n--\nDr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=dgilbert@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xmt3p3SQbz9sD9\n\tfor ;\n\tWed, 6 Sep 2017 02:49:42 +1000 (AEST)","from localhost ([::1]:60195 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpH2i-0004YH-Jm\n\tfor incoming@patchwork.ozlabs.org; Tue, 05 Sep 2017 12:49:40 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:59353)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpH1t-0004ML-1j\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 12:48:56 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpH1o-0004C3-2K\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 12:48:49 -0400","from mx1.redhat.com ([209.132.183.28]:50520)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpH1n-0004BW-Op; Tue, 05 Sep 2017 12:48:43 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 81BE36774B;\n\tTue, 5 Sep 2017 16:48:42 +0000 (UTC)","from work-vm (ovpn-117-197.ams2.redhat.com [10.36.117.197])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 6D2587D781;\n\tTue, 5 Sep 2017 16:48:38 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 81BE36774B","Date":"Tue, 5 Sep 2017 17:48:36 +0100","From":"\"Dr. David Alan Gilbert\" ","To":"Markus Armbruster ","Message-ID":"<20170905164835.GF2112@work-vm>","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<871snlwev0.fsf@dusky.pond.sub.org>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tTue, 05 Sep 2017 16:48:42 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [RFC PATCH] tests: Add a device_add/del HMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Huth , Alexander Graf ,\n\tqemu-devel@nongnu.org, qemu-ppc@nongnu.org, Igor Mammedov\n\t, John Snow , Andreas\n\t=?iso-8859-1?q?F=E4rber?= , Philippe\n\t=?iso-8859-1?q?Mathieu-Daud=E9?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1763547,"web_url":"http://patchwork.ozlabs.org/comment/1763547/","msgid":"<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>","list_archive_url":null,"date":"2017-09-05T18:11:39","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":66152,"url":"http://patchwork.ozlabs.org/api/people/66152/","name":"Thomas Huth","email":"thuth@redhat.com"},"content":"On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n> * Markus Armbruster (armbru@redhat.com) wrote:\n>> Thomas Huth writes:\n>>\n>>> People tend to forget to mark internal devices with \"user_creatable = false\n>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n>>> HMP monitor. So let's add a test to run through all devices and that tries\n>>> to add them blindly (without arguments) to see whether this could crash the\n>>> QEMU instance.\n>>>\n>>> Signed-off-by: Thomas Huth \n>>> ---\n>>> I've marked the patch as RFC since not all of the required device bug\n>>> fixes have been merged yet (so this patch can not be included yet without\n>>> breaking \"make check\"). It's also sad that \"macio-oldworld\" currently\n>>> has to be blacklisted - I tried to find a fix for that device, but I was\n>>> not able to spot the exact problem so far. So help for fixing that device\n>>> is very very welcome! The crash can be reproduced like this:\n>>>\n>>> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none\n>>> QEMU 2.10.50 monitor - type 'help' for more information\n>>> (qemu) device_add macio-oldworld,id=x\n>>> (qemu) device_del x\n>>> (qemu) **\n>>> ERROR:qom/object.c:1611:object_get_canonical_path_component:\n>>> assertion failed: (obj->parent != NULL)\n>>> Aborted (core dumped)\n>>>\n>>> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n>>> 1 file changed, 59 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c\n>>> index 7a38cdc..e8a25c4 100644\n>>> --- a/tests/test-hmp.c\n>>> +++ b/tests/test-hmp.c\n>>> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = {\n>>> \"commit all\",\n>>> \"cpu-add 1\",\n>>> \"cpu 0\",\n>>> - \"device_add ?\",\n>>> \"device_add usb-mouse,id=mouse1\",\n>>> \"mouse_button 7\",\n>>> \"mouse_move 10 10\",\n>>> @@ -116,6 +115,64 @@ static void test_info_commands(void)\n>>> g_free(info_buf);\n>>> }\n>>> \n>>> +/*\n>>> + * People tend to forget to mark internal devices with \"user_creatable = false\n>>> + * and these devices can crash QEMU if added via the HMP monitor. So let's run\n>>> + * through all devices and try to add them blindly (without arguments) to see\n>>> + * whether this could crash the QEMU instance.\n>>> + */\n>>> +static void test_device_add_commands(void)\n>>> +{\n>>> + char *resp, *devices, *devices_buf, *endp;\n>>> +\n>>> + devices = devices_buf = hmp(\"device_add help\");\n>>> +\n>>> + while (*devices) {\n>>> + /* Ignore header lines etc. */\n>>> + if (strncmp(devices, \"name \\\"\", 6)) {\n>>> + devices = strchr(devices, '\\n');\n>>> + if (!devices) {\n>>> + break;\n>>> + }\n>>> + devices += 1;\n>>> + continue;\n>>> + }\n>>> + /* Extract the device name, ignore parameters and description */\n>>> + devices += 6;\n>>> + endp = strchr(devices, '\"');\n>>> + g_assert(endp != NULL);\n>>> + *endp = '\\0';\n>>> + /* Check whether it is blacklisted... */\n>>> + if (g_str_equal(\"macio-oldworld\", devices)) {\n>>> + devices = strchr(endp + 1, '\\n');\n>>> + if (!devices) {\n>>> + break;\n>>> + }\n>>> + devices += 1;\n>>> + continue;\n>>> + }\n>>> + /* Now run the device_add + device_del commands */\n>>> + if (verbose) {\n>>> + fprintf(stderr, \"\\tdevice_add %s,id=%s\\n\", devices, devices);\n>>> + }\n>>> + resp = hmp(\"device_add %s,id=%s\", devices, devices);\n>>> + g_free(resp);\n>>> + if (verbose) {\n>>> + fprintf(stderr, \"\\tdevice_del %s\\n\", devices);\n>>> + }\n>>> + resp = hmp(\"device_del %s\", devices);\n>>> + g_free(resp);\n>>> + /* And move forward to the next line */\n>>> + devices = strchr(endp + 1, '\\n');\n>>> + if (!devices) {\n>>> + break;\n>>> + }\n>>> + devices += 1;\n>>> + }\n>>> +\n>>> + g_free(devices_buf);\n>>> +}\n>>> +\n>>> static void test_machine(gconstpointer data)\n>>> {\n>>> const char *machine = data;\n>>> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data)\n>>> qtest_start(args);\n>>> \n>>> test_info_commands();\n>>> + test_device_add_commands();\n>>> test_commands();\n>>> \n>>> qtest_end();\n>>\n>> This finds devices by parsing output of HMP help. I think you should\n>> use introspection instead, like device-introspect-test does. You may\n>> want to extract common utility code from there.\n\nWell, I wrote a HMP test, to simulate what users can do at the HMP\nprompt. But ok, it's likely to crash QEMU also when using the QMP\ninterface instead ... but then the code should also go into a different\n.c file, I think.\n\n>> The actual device_add and device_del also use HMP. Failures are\n>> ignored. A few device_add failures I'd expect:\n>>\n>> * There is no suitable bus.\n>>\n>> * There are suitable buses, but the default one is full.\n\nYou can partly work around the above two problems by looping a couple of\ntimes through the \"device_add\"s first, before doing the \"device_del\"s.\nThen the first iteration adds additional buses which get populated in\nthe second iteration. I can get additional QEMU crashes if I modify my\ntest that way... but currently I lack time for debugging all these\ncrashes, so I don't want to include that in this patch here yet.\n\n>> * Mandatory parameters are missing, such as device backend.\n\nThat's quite hard to handle even with QMP, isn't it? How should a\ngeneric test know which parameter have to be populated with which value?\n\n>> * The bus doesn't support hot plug (some other bus might).\n\nThat should not be a problem - at least QEMU should not crash in this\nsituation.\n\n>> * The device supports only cold plug with -device, not hot plug with\n>> device_add.\n\nWe've got Eduardo's scripts/device-crash-test script for that already,\nso no need to cover that here.\n\n>> I'm afraid the test only tests one of these common failure modes for\n>> many devices.\n>>\n>> device_del failures I'd expect:\n>>\n>> * The device doesn't exist, because it hasn't completed hot plug, yet.\n>> In some cases such as ACPI PCI hot plug, hot plug may require guest\n>> cooperation, which may take unbounded time. device_add merely kicks\n>> off the hot plug then. I can't remember how to poll for completion.\n>> I also can't remember why we don't send a QMP event.\n>>\n>> The hot plug usually completes quickly, but it may take its own sweet\n>> time, or not complete at all, say because the guest doesn't support\n>> ACPI, or just doesn't feel like plugging right now.\n>>\n>> The test needs to set up a guest that cooperates. I guess that\n>> involves libqos; yet another thing I've forgotten.\n\nThat was certainly not my scope when I wrote this test. I just want to\nget rid of these devices that can easily crash QEMU if you just try to\nadd or remove them at the monitor prompt. A more detailed hotplug test\nshould IMHO be done by the individual device tests instead, like it is\ndone in many tests/virtio*.c and tests/usb*.c files already.\n\n>> * Same for device_del. You should wait for the DEVICE_DELETED event\n>> with a suitable timeout.\n> \n> Yes, I think that's an interesting problem - although the test\n> might still be worth it just to see how many issues it finds; I'm\n> curious how many devices actually work with no parameters though,\n> most seem to fail.\n\nMy test already helped to find lots of problems:\n\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=f58f25599b72c7479e6a1\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=469f3da42ef4af347fa78\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=574ee06de9c4fe63c90be\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=84ebd3e8c7d4fe955b359\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=0d4fa4996fc5ee56ea7d0\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=1f98e55385d11da1dc0de\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=8ccccff9dd7ba24c7a788\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=0479097859372a760843a\nhttps://git.qemu.org/?p=qemu.git;a=commitdiff;h=a808c0865b720e22ca292\nhttps://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04618.html\nhttps://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04164.html\nhttps://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00306.html\nhttps://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04622.html\nhttps://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04635.html\nhttps://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05390.html\n\n... so does it now sound at least a little bit usable?\n\n> If I'm reading the code right it's creating the device with the same\n> name as the device; I wonder if that always works?\n\nWhy not? The id is just an arbitrary string, isn't it?\n\n> But still, it should mean that if the previous hotplug hasn't really\n> happened then you can move onto the next add.\n> \n>> We could improve device_add to cold plug before the machine starts,\n>> i.e. between start with -S and the first cont. We could similarly\n>> improve device_del to cold plug. Together, that would let you sidestep\n>> the hot plug complications.\n>>\n>> I guess this test is a case of \"if it was easy, we would've done it\n>> already\"...\n> \n> Still maybe it's worth it as a start.\n\nAgreed that we should finally move to a smarter, QMP-based test. But\nuntil someone wrote that, maybe we could include this as a temporary\nguard to avoid that problems like the ones above creep in again?\n\n Thomas","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=thuth@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xmvvD2yZ3z9t16\n\tfor ;\n\tWed, 6 Sep 2017 04:12:23 +1000 (AEST)","from localhost ([::1]:60500 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpIKi-0008K9-Op\n\tfor incoming@patchwork.ozlabs.org; Tue, 05 Sep 2017 14:12:20 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35570)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpIKK-0008Ia-7R\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 14:12:02 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpIKE-0001lj-A5\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 14:11:56 -0400","from mx1.redhat.com ([209.132.183.28]:36402)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpIKE-0001l5-0o; Tue, 05 Sep 2017 14:11:50 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 15B80C08E280;\n\tTue, 5 Sep 2017 18:11:49 +0000 (UTC)","from [10.36.116.114] (ovpn-116-114.ams2.redhat.com [10.36.116.114])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id D142F6EF6A;\n\tTue, 5 Sep 2017 18:11:40 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 15B80C08E280","To":"\"Dr. David Alan Gilbert\" ,\n\tMarkus Armbruster ","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>","From":"Thomas Huth ","Message-ID":"<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>","Date":"Tue, 5 Sep 2017 20:11:39 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170905164835.GF2112@work-vm>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]);\n\tTue, 05 Sep 2017 18:11:49 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Eduardo Habkost , =?utf-8?q?Philippe_Mathieu-Daud?=\n\t=?utf-8?b?w6k=?= ,\n\tqemu-devel@nongnu.org, qemu-ppc@nongnu.org, Igor Mammedov\n\t, John Snow , =?utf-8?q?Andrea?=\n\t=?utf-8?b?cyBGw6RyYmVy?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1763559,"web_url":"http://patchwork.ozlabs.org/comment/1763559/","msgid":"<20170905183716.GH2112@work-vm>","list_archive_url":null,"date":"2017-09-05T18:37:17","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":48102,"url":"http://patchwork.ozlabs.org/api/people/48102/","name":"Dr. David Alan Gilbert","email":"dgilbert@redhat.com"},"content":"* Thomas Huth (thuth@redhat.com) wrote:\n> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n> > * Markus Armbruster (armbru@redhat.com) wrote:\n> >> Thomas Huth writes:\n> >>\n> >>> People tend to forget to mark internal devices with \"user_creatable = false\n> >>> or hotpluggable = false, and these devices can crash QEMU if added via the\n> >>> HMP monitor. So let's add a test to run through all devices and that tries\n> >>> to add them blindly (without arguments) to see whether this could crash the\n> >>> QEMU instance.\n> >>>\n> >>> Signed-off-by: Thomas Huth \n> >>> ---\n> >>> I've marked the patch as RFC since not all of the required device bug\n> >>> fixes have been merged yet (so this patch can not be included yet without\n> >>> breaking \"make check\"). It's also sad that \"macio-oldworld\" currently\n> >>> has to be blacklisted - I tried to find a fix for that device, but I was\n> >>> not able to spot the exact problem so far. So help for fixing that device\n> >>> is very very welcome! The crash can be reproduced like this:\n> >>>\n> >>> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none\n> >>> QEMU 2.10.50 monitor - type 'help' for more information\n> >>> (qemu) device_add macio-oldworld,id=x\n> >>> (qemu) device_del x\n> >>> (qemu) **\n> >>> ERROR:qom/object.c:1611:object_get_canonical_path_component:\n> >>> assertion failed: (obj->parent != NULL)\n> >>> Aborted (core dumped)\n> >>>\n> >>> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> >>> 1 file changed, 59 insertions(+), 1 deletion(-)\n> >>>\n> >>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c\n> >>> index 7a38cdc..e8a25c4 100644\n> >>> --- a/tests/test-hmp.c\n> >>> +++ b/tests/test-hmp.c\n> >>> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = {\n> >>> \"commit all\",\n> >>> \"cpu-add 1\",\n> >>> \"cpu 0\",\n> >>> - \"device_add ?\",\n> >>> \"device_add usb-mouse,id=mouse1\",\n> >>> \"mouse_button 7\",\n> >>> \"mouse_move 10 10\",\n> >>> @@ -116,6 +115,64 @@ static void test_info_commands(void)\n> >>> g_free(info_buf);\n> >>> }\n> >>> \n> >>> +/*\n> >>> + * People tend to forget to mark internal devices with \"user_creatable = false\n> >>> + * and these devices can crash QEMU if added via the HMP monitor. So let's run\n> >>> + * through all devices and try to add them blindly (without arguments) to see\n> >>> + * whether this could crash the QEMU instance.\n> >>> + */\n> >>> +static void test_device_add_commands(void)\n> >>> +{\n> >>> + char *resp, *devices, *devices_buf, *endp;\n> >>> +\n> >>> + devices = devices_buf = hmp(\"device_add help\");\n> >>> +\n> >>> + while (*devices) {\n> >>> + /* Ignore header lines etc. */\n> >>> + if (strncmp(devices, \"name \\\"\", 6)) {\n> >>> + devices = strchr(devices, '\\n');\n> >>> + if (!devices) {\n> >>> + break;\n> >>> + }\n> >>> + devices += 1;\n> >>> + continue;\n> >>> + }\n> >>> + /* Extract the device name, ignore parameters and description */\n> >>> + devices += 6;\n> >>> + endp = strchr(devices, '\"');\n> >>> + g_assert(endp != NULL);\n> >>> + *endp = '\\0';\n> >>> + /* Check whether it is blacklisted... */\n> >>> + if (g_str_equal(\"macio-oldworld\", devices)) {\n> >>> + devices = strchr(endp + 1, '\\n');\n> >>> + if (!devices) {\n> >>> + break;\n> >>> + }\n> >>> + devices += 1;\n> >>> + continue;\n> >>> + }\n> >>> + /* Now run the device_add + device_del commands */\n> >>> + if (verbose) {\n> >>> + fprintf(stderr, \"\\tdevice_add %s,id=%s\\n\", devices, devices);\n> >>> + }\n> >>> + resp = hmp(\"device_add %s,id=%s\", devices, devices);\n> >>> + g_free(resp);\n> >>> + if (verbose) {\n> >>> + fprintf(stderr, \"\\tdevice_del %s\\n\", devices);\n> >>> + }\n> >>> + resp = hmp(\"device_del %s\", devices);\n> >>> + g_free(resp);\n> >>> + /* And move forward to the next line */\n> >>> + devices = strchr(endp + 1, '\\n');\n> >>> + if (!devices) {\n> >>> + break;\n> >>> + }\n> >>> + devices += 1;\n> >>> + }\n> >>> +\n> >>> + g_free(devices_buf);\n> >>> +}\n> >>> +\n> >>> static void test_machine(gconstpointer data)\n> >>> {\n> >>> const char *machine = data;\n> >>> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data)\n> >>> qtest_start(args);\n> >>> \n> >>> test_info_commands();\n> >>> + test_device_add_commands();\n> >>> test_commands();\n> >>> \n> >>> qtest_end();\n> >>\n> >> This finds devices by parsing output of HMP help. I think you should\n> >> use introspection instead, like device-introspect-test does. You may\n> >> want to extract common utility code from there.\n> \n> Well, I wrote a HMP test, to simulate what users can do at the HMP\n> prompt. But ok, it's likely to crash QEMU also when using the QMP\n> interface instead ... but then the code should also go into a different\n> .c file, I think.\n> \n> >> The actual device_add and device_del also use HMP. Failures are\n> >> ignored. A few device_add failures I'd expect:\n> >>\n> >> * There is no suitable bus.\n> >>\n> >> * There are suitable buses, but the default one is full.\n> \n> You can partly work around the above two problems by looping a couple of\n> times through the \"device_add\"s first, before doing the \"device_del\"s.\n> Then the first iteration adds additional buses which get populated in\n> the second iteration. I can get additional QEMU crashes if I modify my\n> test that way... but currently I lack time for debugging all these\n> crashes, so I don't want to include that in this patch here yet.\n> \n> >> * Mandatory parameters are missing, such as device backend.\n> \n> That's quite hard to handle even with QMP, isn't it? How should a\n> generic test know which parameter have to be populated with which value?\n> \n> >> * The bus doesn't support hot plug (some other bus might).\n> \n> That should not be a problem - at least QEMU should not crash in this\n> situation.\n> \n> >> * The device supports only cold plug with -device, not hot plug with\n> >> device_add.\n> \n> We've got Eduardo's scripts/device-crash-test script for that already,\n> so no need to cover that here.\n> \n> >> I'm afraid the test only tests one of these common failure modes for\n> >> many devices.\n> >>\n> >> device_del failures I'd expect:\n> >>\n> >> * The device doesn't exist, because it hasn't completed hot plug, yet.\n> >> In some cases such as ACPI PCI hot plug, hot plug may require guest\n> >> cooperation, which may take unbounded time. device_add merely kicks\n> >> off the hot plug then. I can't remember how to poll for completion.\n> >> I also can't remember why we don't send a QMP event.\n> >>\n> >> The hot plug usually completes quickly, but it may take its own sweet\n> >> time, or not complete at all, say because the guest doesn't support\n> >> ACPI, or just doesn't feel like plugging right now.\n> >>\n> >> The test needs to set up a guest that cooperates. I guess that\n> >> involves libqos; yet another thing I've forgotten.\n> \n> That was certainly not my scope when I wrote this test. I just want to\n> get rid of these devices that can easily crash QEMU if you just try to\n> add or remove them at the monitor prompt. A more detailed hotplug test\n> should IMHO be done by the individual device tests instead, like it is\n> done in many tests/virtio*.c and tests/usb*.c files already.\n> \n> >> * Same for device_del. You should wait for the DEVICE_DELETED event\n> >> with a suitable timeout.\n> > \n> > Yes, I think that's an interesting problem - although the test\n> > might still be worth it just to see how many issues it finds; I'm\n> > curious how many devices actually work with no parameters though,\n> > most seem to fail.\n> \n> My test already helped to find lots of problems:\n> \n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=f58f25599b72c7479e6a1\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=469f3da42ef4af347fa78\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=574ee06de9c4fe63c90be\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=84ebd3e8c7d4fe955b359\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0d4fa4996fc5ee56ea7d0\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=1f98e55385d11da1dc0de\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8ccccff9dd7ba24c7a788\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0479097859372a760843a\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a808c0865b720e22ca292\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04618.html\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04164.html\n> https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00306.html\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04622.html\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04635.html\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05390.html\n> \n> ... so does it now sound at least a little bit usable?\n\nSeems a good haul; and a good justification for including it.\n\n> > If I'm reading the code right it's creating the device with the same\n> > name as the device; I wonder if that always works?\n> \n> Why not? The id is just an arbitrary string, isn't it?\n\nI didn't know how arbitrary they were allowed to be and I was\nalso worried they might clash with some existing id.\nAs an example, I see there's at least one device (SUNW,fdtwo) with\na , in it's name - is that legal for an id ?\n\n> \n> > But still, it should mean that if the previous hotplug hasn't really\n> > happened then you can move onto the next add.\n> > \n> >> We could improve device_add to cold plug before the machine starts,\n> >> i.e. between start with -S and the first cont. We could similarly\n> >> improve device_del to cold plug. Together, that would let you sidestep\n> >> the hot plug complications.\n> >>\n> >> I guess this test is a case of \"if it was easy, we would've done it\n> >> already\"...\n> > \n> > Still maybe it's worth it as a start.\n> \n> Agreed that we should finally move to a smarter, QMP-based test. But\n> until someone wrote that, maybe we could include this as a temporary\n> guard to avoid that problems like the ones above creep in again?\n\nIt seems like a good start.\n\nDave\n\n> Thomas\n--\nDr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=dgilbert@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xmwSs6tFWz9sR9\n\tfor ;\n\tWed, 6 Sep 2017 04:38:05 +1000 (AEST)","from localhost ([::1]:60605 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpIjb-0006Z5-Ru\n\tfor incoming@patchwork.ozlabs.org; Tue, 05 Sep 2017 14:38:03 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:44823)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpIj5-0006Yp-QK\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 14:37:37 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpIiz-0007Mw-Ef\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 14:37:31 -0400","from mx1.redhat.com ([209.132.183.28]:39848)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpIiz-0007M0-4d; Tue, 05 Sep 2017 14:37:25 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id EE594C07D8CB;\n\tTue, 5 Sep 2017 18:37:23 +0000 (UTC)","from work-vm (ovpn-117-197.ams2.redhat.com [10.36.117.197])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id A6EC05D6A3;\n\tTue, 5 Sep 2017 18:37:19 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com EE594C07D8CB","Date":"Tue, 5 Sep 2017 19:37:17 +0100","From":"\"Dr. David Alan Gilbert\" ","To":"Thomas Huth ","Message-ID":"<20170905183716.GH2112@work-vm>","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tTue, 05 Sep 2017 18:37:24 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Eduardo Habkost , Markus Armbruster\n\t, qemu-devel@nongnu.org, qemu-ppc@nongnu.org,\n\tIgor Mammedov , \n\tJohn Snow , Andreas =?iso-8859-1?q?F=E4rber?=\n\t, Philippe =?iso-8859-1?q?Mathieu-Daud=E9?=\n\t","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1763809,"web_url":"http://patchwork.ozlabs.org/comment/1763809/","msgid":"<7fd704ae-81c5-dc9b-33f0-fc640399dbf8@redhat.com>","list_archive_url":null,"date":"2017-09-06T04:53:41","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":66152,"url":"http://patchwork.ozlabs.org/api/people/66152/","name":"Thomas Huth","email":"thuth@redhat.com"},"content":"On 05.09.2017 20:37, Dr. David Alan Gilbert wrote:\n> * Thomas Huth (thuth@redhat.com) wrote:\n>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n>>> * Markus Armbruster (armbru@redhat.com) wrote:\n>>>> Thomas Huth writes:\n>>>>\n>>>>> People tend to forget to mark internal devices with \"user_creatable = false\n>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n>>>>> HMP monitor. So let's add a test to run through all devices and that tries\n>>>>> to add them blindly (without arguments) to see whether this could crash the\n>>>>> QEMU instance.\n>>>>>\n>>>>> Signed-off-by: Thomas Huth \n>>>>> ---\n[...]\n>>> If I'm reading the code right it's creating the device with the same\n>>> name as the device; I wonder if that always works?\n>>\n>> Why not? The id is just an arbitrary string, isn't it?\n> \n> I didn't know how arbitrary they were allowed to be and I was\n> also worried they might clash with some existing id.\n> As an example, I see there's at least one device (SUNW,fdtwo) with\n> a , in it's name - is that legal for an id ?\n\nOh, right, I didn't think of comma :-/ ... I'll try to come up with a\nbetter solution in the next version of the patch...\n\n Thomas","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=thuth@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnB7x4gMXz9sBW\n\tfor ;\n\tWed, 6 Sep 2017 14:54:21 +1000 (AEST)","from localhost ([::1]:33996 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpSLz-0005L1-PN\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Sep 2017 00:54:19 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35268)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpSLY-0005Kp-9o\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 00:53:53 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpSLV-0003CW-7R\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 00:53:52 -0400","from mx1.redhat.com ([209.132.183.28]:48978)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpSLV-0003CJ-0a; Wed, 06 Sep 2017 00:53:49 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id B41E983F3E;\n\tWed, 6 Sep 2017 04:53:47 +0000 (UTC)","from [10.36.116.90] (ovpn-116-90.ams2.redhat.com [10.36.116.90])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 9589760F91;\n\tWed, 6 Sep 2017 04:53:43 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com B41E983F3E","To":"\"Dr. David Alan Gilbert\" ","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>\n\t<20170905183716.GH2112@work-vm>","From":"Thomas Huth ","Message-ID":"<7fd704ae-81c5-dc9b-33f0-fc640399dbf8@redhat.com>","Date":"Wed, 6 Sep 2017 06:53:41 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170905183716.GH2112@work-vm>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tWed, 06 Sep 2017 04:53:48 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Eduardo Habkost , Markus Armbruster\n\t, qemu-devel@nongnu.org, qemu-ppc@nongnu.org,\n\tIgor Mammedov , \tJohn Snow ,\n\t=?utf-8?q?Andreas_F=C3=A4rber?= ,\n\t=?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1763844,"web_url":"http://patchwork.ozlabs.org/comment/1763844/","msgid":"<87mv68e2i3.fsf@dusky.pond.sub.org>","list_archive_url":null,"date":"2017-09-06T06:59:32","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":2645,"url":"http://patchwork.ozlabs.org/api/people/2645/","name":"Markus Armbruster","email":"armbru@redhat.com"},"content":"Thomas Huth writes:\n\n> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n>> * Markus Armbruster (armbru@redhat.com) wrote:\n>>> Thomas Huth writes:\n>>>\n>>>> People tend to forget to mark internal devices with \"user_creatable = false\n>>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n>>>> HMP monitor. So let's add a test to run through all devices and that tries\n>>>> to add them blindly (without arguments) to see whether this could crash the\n>>>> QEMU instance.\n>>>>\n>>>> Signed-off-by: Thomas Huth \n>>>> ---\n>>>> I've marked the patch as RFC since not all of the required device bug\n>>>> fixes have been merged yet (so this patch can not be included yet without\n>>>> breaking \"make check\"). It's also sad that \"macio-oldworld\" currently\n>>>> has to be blacklisted - I tried to find a fix for that device, but I was\n>>>> not able to spot the exact problem so far. So help for fixing that device\n>>>> is very very welcome! The crash can be reproduced like this:\n>>>>\n>>>> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none\n>>>> QEMU 2.10.50 monitor - type 'help' for more information\n>>>> (qemu) device_add macio-oldworld,id=x\n>>>> (qemu) device_del x\n>>>> (qemu) **\n>>>> ERROR:qom/object.c:1611:object_get_canonical_path_component:\n>>>> assertion failed: (obj->parent != NULL)\n>>>> Aborted (core dumped)\n>>>>\n>>>> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n>>>> 1 file changed, 59 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c\n>>>> index 7a38cdc..e8a25c4 100644\n>>>> --- a/tests/test-hmp.c\n>>>> +++ b/tests/test-hmp.c\n>>>> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = {\n>>>> \"commit all\",\n>>>> \"cpu-add 1\",\n>>>> \"cpu 0\",\n>>>> - \"device_add ?\",\n>>>> \"device_add usb-mouse,id=mouse1\",\n>>>> \"mouse_button 7\",\n>>>> \"mouse_move 10 10\",\n>>>> @@ -116,6 +115,64 @@ static void test_info_commands(void)\n>>>> g_free(info_buf);\n>>>> }\n>>>> \n>>>> +/*\n>>>> + * People tend to forget to mark internal devices with \"user_creatable = false\n>>>> + * and these devices can crash QEMU if added via the HMP monitor. So let's run\n>>>> + * through all devices and try to add them blindly (without arguments) to see\n>>>> + * whether this could crash the QEMU instance.\n>>>> + */\n>>>> +static void test_device_add_commands(void)\n>>>> +{\n>>>> + char *resp, *devices, *devices_buf, *endp;\n>>>> +\n>>>> + devices = devices_buf = hmp(\"device_add help\");\n>>>> +\n>>>> + while (*devices) {\n>>>> + /* Ignore header lines etc. */\n>>>> + if (strncmp(devices, \"name \\\"\", 6)) {\n>>>> + devices = strchr(devices, '\\n');\n>>>> + if (!devices) {\n>>>> + break;\n>>>> + }\n>>>> + devices += 1;\n>>>> + continue;\n>>>> + }\n>>>> + /* Extract the device name, ignore parameters and description */\n>>>> + devices += 6;\n>>>> + endp = strchr(devices, '\"');\n>>>> + g_assert(endp != NULL);\n>>>> + *endp = '\\0';\n>>>> + /* Check whether it is blacklisted... */\n>>>> + if (g_str_equal(\"macio-oldworld\", devices)) {\n>>>> + devices = strchr(endp + 1, '\\n');\n>>>> + if (!devices) {\n>>>> + break;\n>>>> + }\n>>>> + devices += 1;\n>>>> + continue;\n>>>> + }\n>>>> + /* Now run the device_add + device_del commands */\n>>>> + if (verbose) {\n>>>> + fprintf(stderr, \"\\tdevice_add %s,id=%s\\n\", devices, devices);\n>>>> + }\n>>>> + resp = hmp(\"device_add %s,id=%s\", devices, devices);\n>>>> + g_free(resp);\n>>>> + if (verbose) {\n>>>> + fprintf(stderr, \"\\tdevice_del %s\\n\", devices);\n>>>> + }\n>>>> + resp = hmp(\"device_del %s\", devices);\n>>>> + g_free(resp);\n>>>> + /* And move forward to the next line */\n>>>> + devices = strchr(endp + 1, '\\n');\n>>>> + if (!devices) {\n>>>> + break;\n>>>> + }\n>>>> + devices += 1;\n>>>> + }\n>>>> +\n>>>> + g_free(devices_buf);\n>>>> +}\n>>>> +\n>>>> static void test_machine(gconstpointer data)\n>>>> {\n>>>> const char *machine = data;\n>>>> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data)\n>>>> qtest_start(args);\n>>>> \n>>>> test_info_commands();\n>>>> + test_device_add_commands();\n>>>> test_commands();\n>>>> \n>>>> qtest_end();\n>>>\n>>> This finds devices by parsing output of HMP help. I think you should\n>>> use introspection instead, like device-introspect-test does. You may\n>>> want to extract common utility code from there.\n>\n> Well, I wrote a HMP test, to simulate what users can do at the HMP\n> prompt. But ok, it's likely to crash QEMU also when using the QMP\n> interface instead ... but then the code should also go into a different\n> .c file, I think.\n\nHMP device_add is a trivial wrapper around QMP, as is proper:\n\n void hmp_device_add(Monitor *mon, const QDict *qdict)\n {\n Error *err = NULL;\n\n qmp_device_add((QDict *)qdict, NULL, &err);\n hmp_handle_error(mon, &err);\n }\n\nIf anything ever breaks in this wrapper, it won't be specific to HMP\ndevice_add.\n\n>>> The actual device_add and device_del also use HMP. Failures are\n>>> ignored. A few device_add failures I'd expect:\n>>>\n>>> * There is no suitable bus.\n>>>\n>>> * There are suitable buses, but the default one is full.\n>\n> You can partly work around the above two problems by looping a couple of\n> times through the \"device_add\"s first, before doing the \"device_del\"s.\n> Then the first iteration adds additional buses which get populated in\n> the second iteration. I can get additional QEMU crashes if I modify my\n> test that way... but currently I lack time for debugging all these\n> crashes, so I don't want to include that in this patch here yet.\n\nKind of a random walk.\n\nEduardo has been working on \"socket introspection\", i.e. means to find\navailable \"sockets\" for devices to plug into. Could be used to for a\nmore directed walk. Eduardo, any thoughts?\n\n>>> * Mandatory parameters are missing, such as device backend.\n>\n> That's quite hard to handle even with QMP, isn't it? How should a\n> generic test know which parameter have to be populated with which value?\n\nIt could perhaps populate sufficiently generic parameters such as common\nbackends.\n\nSadly, device-list-properties is weak, and unless we improve or replace\nit, this would involve somewhat ugly hardcoding of descriptions. For\ninstance, we'd have to recognize from\n\n {\"name\": \"netdev\", \"description\": \"ID of a netdev to use as a backend\", \"type\": \"str\"}\n\nthat property netdev is a network backend, and very likely mandatory.\n\n>>> * The bus doesn't support hot plug (some other bus might).\n>\n> That should not be a problem - at least QEMU should not crash in this\n> situation.\n\nYes, and testing that has some value. It doesn't test the device,\nthough, only the qdev core.\n\nI don't mean to suggest your test is useless. I'm merely pointing at\nsome gaping coverage holes :)\n\n>>> * The device supports only cold plug with -device, not hot plug with\n>>> device_add.\n>\n> We've got Eduardo's scripts/device-crash-test script for that already,\n> so no need to cover that here.\n\nPoint taken. So this test is really about hot plug / unplug. Suggest\nto clarify the commit message: s/add them blindly/hotplug and unplug\nthem blindly/.\n\n>>> I'm afraid the test only tests one of these common failure modes for\n>>> many devices.\n>>>\n>>> device_del failures I'd expect:\n>>>\n>>> * The device doesn't exist, because it hasn't completed hot plug, yet.\n>>> In some cases such as ACPI PCI hot plug, hot plug may require guest\n>>> cooperation, which may take unbounded time. device_add merely kicks\n>>> off the hot plug then. I can't remember how to poll for completion.\n>>> I also can't remember why we don't send a QMP event.\n>>>\n>>> The hot plug usually completes quickly, but it may take its own sweet\n>>> time, or not complete at all, say because the guest doesn't support\n>>> ACPI, or just doesn't feel like plugging right now.\n>>>\n>>> The test needs to set up a guest that cooperates. I guess that\n>>> involves libqos; yet another thing I've forgotten.\n>\n> That was certainly not my scope when I wrote this test. I just want to\n> get rid of these devices that can easily crash QEMU if you just try to\n> add or remove them at the monitor prompt. A more detailed hotplug test\n> should IMHO be done by the individual device tests instead, like it is\n> done in many tests/virtio*.c and tests/usb*.c files already.\n\nI'm not trying to talk you into widening the scope of your test. I am\npointing out that your testing of unplug is racy, and may therefore miss\nfailures randomly: if hotplug is slow, device_del won't actually do\nanything interesting.\n\n>>> * Same for device_del. You should wait for the DEVICE_DELETED event\n>>> with a suitable timeout.\n>> \n>> Yes, I think that's an interesting problem - although the test\n>> might still be worth it just to see how many issues it finds; I'm\n>> curious how many devices actually work with no parameters though,\n>> most seem to fail.\n>\n> My test already helped to find lots of problems:\n>\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=f58f25599b72c7479e6a1\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=469f3da42ef4af347fa78\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=574ee06de9c4fe63c90be\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=84ebd3e8c7d4fe955b359\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0d4fa4996fc5ee56ea7d0\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=1f98e55385d11da1dc0de\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8ccccff9dd7ba24c7a788\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0479097859372a760843a\n> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a808c0865b720e22ca292\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04618.html\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04164.html\n> https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00306.html\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04622.html\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04635.html\n> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05390.html\n>\n> ... so does it now sound at least a little bit usable?\n\nI don't doubt it is, but its limitations need to be understood and\neither relaxed or documented.\n\n>> If I'm reading the code right it's creating the device with the same\n>> name as the device; I wonder if that always works?\n>\n> Why not? The id is just an arbitrary string, isn't it?\n\nSince you're using HMP, you get to quote ',', which occurs in some\ndevice names[*]. Enjoy! ;-P\n\nPicking IDs that aren't anti-social may be easier.\n\n>> But still, it should mean that if the previous hotplug hasn't really\n>> happened then you can move onto the next add.\n>> \n>>> We could improve device_add to cold plug before the machine starts,\n>>> i.e. between start with -S and the first cont. We could similarly\n>>> improve device_del to cold plug. Together, that would let you sidestep\n>>> the hot plug complications.\n>>>\n>>> I guess this test is a case of \"if it was easy, we would've done it\n>>> already\"...\n>> \n>> Still maybe it's worth it as a start.\n>\n> Agreed that we should finally move to a smarter, QMP-based test. But\n> until someone wrote that, maybe we could include this as a temporary\n> guard to avoid that problems like the ones above creep in again?\n\nI think its limitations need to be understood to a useful degree, and\neither relaxed or documented.\n\n\n[*] Blame IEEE 1275 device trees.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=armbru@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnDx958rPz9sRV\n\tfor ;\n\tWed, 6 Sep 2017 17:00:13 +1000 (AEST)","from localhost ([::1]:34429 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpUJn-0003IE-RV\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Sep 2017 03:00:11 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:44240)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpUJI-0003Hz-7z\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 02:59:42 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpUJF-00055A-35\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 02:59:40 -0400","from mx1.redhat.com ([209.132.183.28]:54636)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpUJE-00054f-QE; Wed, 06 Sep 2017 02:59:37 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 8A3687E421;\n\tWed, 6 Sep 2017 06:59:35 +0000 (UTC)","from blackfin.pond.sub.org (ovpn-116-75.ams2.redhat.com\n\t[10.36.116.75])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 9F2366FAAB;\n\tWed, 6 Sep 2017 06:59:33 +0000 (UTC)","by blackfin.pond.sub.org (Postfix, from userid 1000)\n\tid 164B01138645; Wed, 6 Sep 2017 08:59:32 +0200 (CEST)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 8A3687E421","From":"Markus Armbruster ","To":"Thomas Huth ","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>","Date":"Wed, 06 Sep 2017 08:59:32 +0200","In-Reply-To":"<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com> (Thomas Huth's\n\tmessage of \"Tue, 5 Sep 2017 20:11:39 +0200\")","Message-ID":"<87mv68e2i3.fsf@dusky.pond.sub.org>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tWed, 06 Sep 2017 06:59:35 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Eduardo Habkost , qemu-devel@nongnu.org, Philippe\n\t=?utf-8?q?Mathieu-Daud=C3=A9?= , qemu-ppc@nongnu.org,\n\tIgor Mammedov , John Snow ,\n\tAndreas =?utf-8?q?F=C3=A4rber?= ,\n\t\"Dr. David Alan Gilbert\" ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1765860,"web_url":"http://patchwork.ozlabs.org/comment/1765860/","msgid":"<20170909204133.GH7570@localhost.localdomain>","list_archive_url":null,"date":"2017-09-09T20:41:33","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":195,"url":"http://patchwork.ozlabs.org/api/people/195/","name":"Eduardo Habkost","email":"ehabkost@redhat.com"},"content":"On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote:\n> Thomas Huth writes:\n> \n> > On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n> >> * Markus Armbruster (armbru@redhat.com) wrote:\n> >>> Thomas Huth writes:\n> >>>\n> >>>> People tend to forget to mark internal devices with \"user_creatable = false\n> >>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n> >>>> HMP monitor. So let's add a test to run through all devices and that tries\n> >>>> to add them blindly (without arguments) to see whether this could crash the\n> >>>> QEMU instance.\n> >>>>\n> >>>> Signed-off-by: Thomas Huth \n> >>>> ---\n> >>>> I've marked the patch as RFC since not all of the required device bug\n> >>>> fixes have been merged yet (so this patch can not be included yet without\n> >>>> breaking \"make check\"). It's also sad that \"macio-oldworld\" currently\n> >>>> has to be blacklisted - I tried to find a fix for that device, but I was\n> >>>> not able to spot the exact problem so far. So help for fixing that device\n> >>>> is very very welcome! The crash can be reproduced like this:\n> >>>>\n> >>>> $ qemu-system-ppc64 -nographic -S -monitor stdio -serial none\n> >>>> QEMU 2.10.50 monitor - type 'help' for more information\n> >>>> (qemu) device_add macio-oldworld,id=x\n> >>>> (qemu) device_del x\n> >>>> (qemu) **\n> >>>> ERROR:qom/object.c:1611:object_get_canonical_path_component:\n> >>>> assertion failed: (obj->parent != NULL)\n> >>>> Aborted (core dumped)\n> >>>>\n> >>>> tests/test-hmp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> >>>> 1 file changed, 59 insertions(+), 1 deletion(-)\n> >>>>\n> >>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c\n> >>>> index 7a38cdc..e8a25c4 100644\n> >>>> --- a/tests/test-hmp.c\n> >>>> +++ b/tests/test-hmp.c\n> >>>> @@ -28,7 +28,6 @@ static const char *hmp_cmds[] = {\n> >>>> \"commit all\",\n> >>>> \"cpu-add 1\",\n> >>>> \"cpu 0\",\n> >>>> - \"device_add ?\",\n> >>>> \"device_add usb-mouse,id=mouse1\",\n> >>>> \"mouse_button 7\",\n> >>>> \"mouse_move 10 10\",\n> >>>> @@ -116,6 +115,64 @@ static void test_info_commands(void)\n> >>>> g_free(info_buf);\n> >>>> }\n> >>>> \n> >>>> +/*\n> >>>> + * People tend to forget to mark internal devices with \"user_creatable = false\n> >>>> + * and these devices can crash QEMU if added via the HMP monitor. So let's run\n> >>>> + * through all devices and try to add them blindly (without arguments) to see\n> >>>> + * whether this could crash the QEMU instance.\n> >>>> + */\n> >>>> +static void test_device_add_commands(void)\n> >>>> +{\n> >>>> + char *resp, *devices, *devices_buf, *endp;\n> >>>> +\n> >>>> + devices = devices_buf = hmp(\"device_add help\");\n> >>>> +\n> >>>> + while (*devices) {\n> >>>> + /* Ignore header lines etc. */\n> >>>> + if (strncmp(devices, \"name \\\"\", 6)) {\n> >>>> + devices = strchr(devices, '\\n');\n> >>>> + if (!devices) {\n> >>>> + break;\n> >>>> + }\n> >>>> + devices += 1;\n> >>>> + continue;\n> >>>> + }\n> >>>> + /* Extract the device name, ignore parameters and description */\n> >>>> + devices += 6;\n> >>>> + endp = strchr(devices, '\"');\n> >>>> + g_assert(endp != NULL);\n> >>>> + *endp = '\\0';\n> >>>> + /* Check whether it is blacklisted... */\n> >>>> + if (g_str_equal(\"macio-oldworld\", devices)) {\n> >>>> + devices = strchr(endp + 1, '\\n');\n> >>>> + if (!devices) {\n> >>>> + break;\n> >>>> + }\n> >>>> + devices += 1;\n> >>>> + continue;\n> >>>> + }\n> >>>> + /* Now run the device_add + device_del commands */\n> >>>> + if (verbose) {\n> >>>> + fprintf(stderr, \"\\tdevice_add %s,id=%s\\n\", devices, devices);\n> >>>> + }\n> >>>> + resp = hmp(\"device_add %s,id=%s\", devices, devices);\n> >>>> + g_free(resp);\n> >>>> + if (verbose) {\n> >>>> + fprintf(stderr, \"\\tdevice_del %s\\n\", devices);\n> >>>> + }\n> >>>> + resp = hmp(\"device_del %s\", devices);\n> >>>> + g_free(resp);\n> >>>> + /* And move forward to the next line */\n> >>>> + devices = strchr(endp + 1, '\\n');\n> >>>> + if (!devices) {\n> >>>> + break;\n> >>>> + }\n> >>>> + devices += 1;\n> >>>> + }\n> >>>> +\n> >>>> + g_free(devices_buf);\n> >>>> +}\n> >>>> +\n> >>>> static void test_machine(gconstpointer data)\n> >>>> {\n> >>>> const char *machine = data;\n> >>>> @@ -125,6 +182,7 @@ static void test_machine(gconstpointer data)\n> >>>> qtest_start(args);\n> >>>> \n> >>>> test_info_commands();\n> >>>> + test_device_add_commands();\n> >>>> test_commands();\n> >>>> \n> >>>> qtest_end();\n> >>>\n> >>> This finds devices by parsing output of HMP help. I think you should\n> >>> use introspection instead, like device-introspect-test does. You may\n> >>> want to extract common utility code from there.\n> >\n> > Well, I wrote a HMP test, to simulate what users can do at the HMP\n> > prompt. But ok, it's likely to crash QEMU also when using the QMP\n> > interface instead ... but then the code should also go into a different\n> > .c file, I think.\n> \n> HMP device_add is a trivial wrapper around QMP, as is proper:\n> \n> void hmp_device_add(Monitor *mon, const QDict *qdict)\n> {\n> Error *err = NULL;\n> \n> qmp_device_add((QDict *)qdict, NULL, &err);\n> hmp_handle_error(mon, &err);\n> }\n> \n> If anything ever breaks in this wrapper, it won't be specific to HMP\n> device_add.\n> \n> >>> The actual device_add and device_del also use HMP. Failures are\n> >>> ignored. A few device_add failures I'd expect:\n> >>>\n> >>> * There is no suitable bus.\n> >>>\n> >>> * There are suitable buses, but the default one is full.\n> >\n> > You can partly work around the above two problems by looping a couple of\n> > times through the \"device_add\"s first, before doing the \"device_del\"s.\n> > Then the first iteration adds additional buses which get populated in\n> > the second iteration. I can get additional QEMU crashes if I modify my\n> > test that way... but currently I lack time for debugging all these\n> > crashes, so I don't want to include that in this patch here yet.\n> \n> Kind of a random walk.\n> \n> Eduardo has been working on \"socket introspection\", i.e. means to find\n> available \"sockets\" for devices to plug into. Could be used to for a\n> more directed walk. Eduardo, any thoughts?\n\nMy query-device-slots work in progress included a Python script\nthat tried to validate some of the query-device-slots data. It\ncould include one additional check where it tries to plug devices\nto those slots and see if something breaks.\n\nBut considering the way the device hotplug code works today, test\ncode that tries random device-types/arguments that are _not_\nreported by query-device-slots would be useful too.\n\n> \n> >>> * Mandatory parameters are missing, such as device backend.\n> >\n> > That's quite hard to handle even with QMP, isn't it? How should a\n> > generic test know which parameter have to be populated with which value?\n> \n> It could perhaps populate sufficiently generic parameters such as common\n> backends.\n> \n> Sadly, device-list-properties is weak, and unless we improve or replace\n> it, this would involve somewhat ugly hardcoding of descriptions. For\n> instance, we'd have to recognize from\n> \n> {\"name\": \"netdev\", \"description\": \"ID of a netdev to use as a backend\", \"type\": \"str\"}\n> \n> that property netdev is a network backend, and very likely mandatory.\n> \n> >>> * The bus doesn't support hot plug (some other bus might).\n> >\n> > That should not be a problem - at least QEMU should not crash in this\n> > situation.\n> \n> Yes, and testing that has some value. It doesn't test the device,\n> though, only the qdev core.\n> \n> I don't mean to suggest your test is useless. I'm merely pointing at\n> some gaping coverage holes :)\n> \n> >>> * The device supports only cold plug with -device, not hot plug with\n> >>> device_add.\n> >\n> > We've got Eduardo's scripts/device-crash-test script for that already,\n> > so no need to cover that here.\n> \n> Point taken. So this test is really about hot plug / unplug. Suggest\n> to clarify the commit message: s/add them blindly/hotplug and unplug\n> them blindly/.\n\nWe could extend device-crash-test to test device_add too, as it\nalready has extra code to deal with known crashes and testing\nmultiple machine-types. Also, any additional code we write to\nensure we add mandatory arguments or plug only to valid buses\nwould apply to both -device and device_add. I also think Python\ntest code is easier to maintain and extend, but that's just my\npersonal preference.\n\n> \n> >>> I'm afraid the test only tests one of these common failure modes for\n> >>> many devices.\n> >>>\n> >>> device_del failures I'd expect:\n> >>>\n> >>> * The device doesn't exist, because it hasn't completed hot plug, yet.\n> >>> In some cases such as ACPI PCI hot plug, hot plug may require guest\n> >>> cooperation, which may take unbounded time. device_add merely kicks\n> >>> off the hot plug then. I can't remember how to poll for completion.\n> >>> I also can't remember why we don't send a QMP event.\n> >>>\n> >>> The hot plug usually completes quickly, but it may take its own sweet\n> >>> time, or not complete at all, say because the guest doesn't support\n> >>> ACPI, or just doesn't feel like plugging right now.\n> >>>\n> >>> The test needs to set up a guest that cooperates. I guess that\n> >>> involves libqos; yet another thing I've forgotten.\n> >\n> > That was certainly not my scope when I wrote this test. I just want to\n> > get rid of these devices that can easily crash QEMU if you just try to\n> > add or remove them at the monitor prompt. A more detailed hotplug test\n> > should IMHO be done by the individual device tests instead, like it is\n> > done in many tests/virtio*.c and tests/usb*.c files already.\n> \n> I'm not trying to talk you into widening the scope of your test. I am\n> pointing out that your testing of unplug is racy, and may therefore miss\n> failures randomly: if hotplug is slow, device_del won't actually do\n> anything interesting.\n> \n> >>> * Same for device_del. You should wait for the DEVICE_DELETED event\n> >>> with a suitable timeout.\n> >> \n> >> Yes, I think that's an interesting problem - although the test\n> >> might still be worth it just to see how many issues it finds; I'm\n> >> curious how many devices actually work with no parameters though,\n> >> most seem to fail.\n> >\n> > My test already helped to find lots of problems:\n> >\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=f58f25599b72c7479e6a1\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=469f3da42ef4af347fa78\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=574ee06de9c4fe63c90be\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=84ebd3e8c7d4fe955b359\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0d4fa4996fc5ee56ea7d0\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=1f98e55385d11da1dc0de\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8ccccff9dd7ba24c7a788\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0479097859372a760843a\n> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a808c0865b720e22ca292\n> > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04618.html\n> > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04164.html\n> > https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00306.html\n> > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04622.html\n> > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04635.html\n> > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05390.html\n> >\n> > ... so does it now sound at least a little bit usable?\n> \n> I don't doubt it is, but its limitations need to be understood and\n> either relaxed or documented.\n> \n> >> If I'm reading the code right it's creating the device with the same\n> >> name as the device; I wonder if that always works?\n> >\n> > Why not? The id is just an arbitrary string, isn't it?\n> \n> Since you're using HMP, you get to quote ',', which occurs in some\n> device names[*]. Enjoy! ;-P\n> \n> Picking IDs that aren't anti-social may be easier.\n> \n> >> But still, it should mean that if the previous hotplug hasn't really\n> >> happened then you can move onto the next add.\n> >> \n> >>> We could improve device_add to cold plug before the machine starts,\n> >>> i.e. between start with -S and the first cont. We could similarly\n> >>> improve device_del to cold plug. Together, that would let you sidestep\n> >>> the hot plug complications.\n> >>>\n> >>> I guess this test is a case of \"if it was easy, we would've done it\n> >>> already\"...\n> >> \n> >> Still maybe it's worth it as a start.\n> >\n> > Agreed that we should finally move to a smarter, QMP-based test. But\n> > until someone wrote that, maybe we could include this as a temporary\n> > guard to avoid that problems like the ones above creep in again?\n> \n> I think its limitations need to be understood to a useful degree, and\n> either relaxed or documented.\n> \n> \n> [*] Blame IEEE 1275 device trees.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=ehabkost@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xqR273Ntzz9s7g\n\tfor ;\n\tSun, 10 Sep 2017 06:42:05 +1000 (AEST)","from localhost ([::1]:50827 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dqmZm-0003tg-EE\n\tfor incoming@patchwork.ozlabs.org; Sat, 09 Sep 2017 16:42:02 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47199)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dqmZS-0003sX-Td\n\tfor qemu-devel@nongnu.org; Sat, 09 Sep 2017 16:41:45 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dqmZP-0006D2-OG\n\tfor qemu-devel@nongnu.org; Sat, 09 Sep 2017 16:41:42 -0400","from mx1.redhat.com ([209.132.183.28]:56772)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dqmZP-0006BM-ER; Sat, 09 Sep 2017 16:41:39 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 15EEAC04B329;\n\tSat, 9 Sep 2017 20:41:38 +0000 (UTC)","from localhost (ovpn-116-45.gru2.redhat.com [10.97.116.45])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 6C3056F964;\n\tSat, 9 Sep 2017 20:41:35 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 15EEAC04B329","Date":"Sat, 9 Sep 2017 17:41:33 -0300","From":"Eduardo Habkost ","To":"Markus Armbruster ","Message-ID":"<20170909204133.GH7570@localhost.localdomain>","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>\n\t<87mv68e2i3.fsf@dusky.pond.sub.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<87mv68e2i3.fsf@dusky.pond.sub.org>","X-Fnord":"you can see the fnord","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]);\n\tSat, 09 Sep 2017 20:41:38 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Huth , qemu-devel@nongnu.org, \"Dr. David Alan\n\tGilbert\" , qemu-ppc@nongnu.org,\n\tIgor Mammedov , John Snow ,\n\tAndreas =?iso-8859-1?q?F=E4rber?= , Philippe\n\t=?iso-8859-1?q?Mathieu-Daud=E9?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1766053,"web_url":"http://patchwork.ozlabs.org/comment/1766053/","msgid":"<825b29f3-7857-558e-9092-855c9f75463e@redhat.com>","list_archive_url":null,"date":"2017-09-11T06:13:21","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":66152,"url":"http://patchwork.ozlabs.org/api/people/66152/","name":"Thomas Huth","email":"thuth@redhat.com"},"content":"On 09.09.2017 22:41, Eduardo Habkost wrote:\n> On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote:\n>> Thomas Huth writes:\n>>\n>>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n>>>> * Markus Armbruster (armbru@redhat.com) wrote:\n>>>>> Thomas Huth writes:\n>>>>>\n>>>>>> People tend to forget to mark internal devices with \"user_creatable = false\n>>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n>>>>>> HMP monitor. So let's add a test to run through all devices and that tries\n>>>>>> to add them blindly (without arguments) to see whether this could crash the\n>>>>>> QEMU instance.\n[...]\n>>>>> * The device supports only cold plug with -device, not hot plug with\n>>>>> device_add.\n>>>\n>>> We've got Eduardo's scripts/device-crash-test script for that already,\n>>> so no need to cover that here.\n>>\n>> Point taken. So this test is really about hot plug / unplug. Suggest\n>> to clarify the commit message: s/add them blindly/hotplug and unplug\n>> them blindly/.\n> \n> We could extend device-crash-test to test device_add too, as it\n> already has extra code to deal with known crashes and testing\n> multiple machine-types. Also, any additional code we write to\n> ensure we add mandatory arguments or plug only to valid buses\n> would apply to both -device and device_add. I also think Python\n> test code is easier to maintain and extend, but that's just my\n> personal preference.\n\nAdding device_add/del support to device-crash-test is certainly an\noption. The problem is that nobody runs it by default, so this won't\nhelp to avoid that new problems are being committed to the repository.\n\nI think we really should have a test for \"make check\", too. So would my\ntest be acceptable if I'd rewrite it to use QMP instead (I don't think I\ncould do the full list that Markus mentioned, but at least a basic test\nvia QMP as a start)?\n\n>>>> If I'm reading the code right it's creating the device with the same\n>>>> name as the device; I wonder if that always works?\n>>>\n>>> Why not? The id is just an arbitrary string, isn't it?\n>>\n>> Since you're using HMP, you get to quote ',', which occurs in some\n>> device names[*]. Enjoy! ;-P\n>>\n>> Picking IDs that aren't anti-social may be easier.\n\nI'm considering to fail the test if it detects a device with a ',' in\nits name. Such devices should really not be there in QEMU...\n\n Thomas","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=thuth@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xrHgV4S4nz9s76\n\tfor ;\n\tMon, 11 Sep 2017 16:13:57 +1000 (AEST)","from localhost ([::1]:55607 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1drHyj-0007Zs-Pk\n\tfor incoming@patchwork.ozlabs.org; Mon, 11 Sep 2017 02:13:53 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:48958)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drHyO-0007ZY-8o\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 02:13:33 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1drHyK-0005gZ-A8\n\tfor qemu-devel@nongnu.org; Mon, 11 Sep 2017 02:13:32 -0400","from mx1.redhat.com ([209.132.183.28]:59058)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1drHyK-0005gC-1N; Mon, 11 Sep 2017 02:13:28 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id CAE961E2F5;\n\tMon, 11 Sep 2017 06:13:26 +0000 (UTC)","from [10.36.116.113] (ovpn-116-113.ams2.redhat.com [10.36.116.113])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id D122E61982;\n\tMon, 11 Sep 2017 06:13:22 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com CAE961E2F5","To":"Eduardo Habkost ,\n\tMarkus Armbruster ","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>\n\t<87mv68e2i3.fsf@dusky.pond.sub.org>\n\t<20170909204133.GH7570@localhost.localdomain>","From":"Thomas Huth ","Message-ID":"<825b29f3-7857-558e-9092-855c9f75463e@redhat.com>","Date":"Mon, 11 Sep 2017 08:13:21 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170909204133.GH7570@localhost.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.30]);\n\tMon, 11 Sep 2017 06:13:27 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"qemu-devel@nongnu.org, \"Dr. David Alan Gilbert\" ,\n\tqemu-ppc@nongnu.org, Igor Mammedov , John Snow\n\t, =?utf-8?q?Andreas_F=C3=A4rber?= ,\n\t=?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1767265,"web_url":"http://patchwork.ozlabs.org/comment/1767265/","msgid":"<20170912173723.GA7570@localhost.localdomain>","list_archive_url":null,"date":"2017-09-12T17:37:23","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":195,"url":"http://patchwork.ozlabs.org/api/people/195/","name":"Eduardo Habkost","email":"ehabkost@redhat.com"},"content":"On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote:\n> On 09.09.2017 22:41, Eduardo Habkost wrote:\n> > On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote:\n> >> Thomas Huth writes:\n> >>\n> >>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n> >>>> * Markus Armbruster (armbru@redhat.com) wrote:\n> >>>>> Thomas Huth writes:\n> >>>>>\n> >>>>>> People tend to forget to mark internal devices with \"user_creatable = false\n> >>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n> >>>>>> HMP monitor. So let's add a test to run through all devices and that tries\n> >>>>>> to add them blindly (without arguments) to see whether this could crash the\n> >>>>>> QEMU instance.\n> [...]\n> >>>>> * The device supports only cold plug with -device, not hot plug with\n> >>>>> device_add.\n> >>>\n> >>> We've got Eduardo's scripts/device-crash-test script for that already,\n> >>> so no need to cover that here.\n> >>\n> >> Point taken. So this test is really about hot plug / unplug. Suggest\n> >> to clarify the commit message: s/add them blindly/hotplug and unplug\n> >> them blindly/.\n> > \n> > We could extend device-crash-test to test device_add too, as it\n> > already has extra code to deal with known crashes and testing\n> > multiple machine-types. Also, any additional code we write to\n> > ensure we add mandatory arguments or plug only to valid buses\n> > would apply to both -device and device_add. I also think Python\n> > test code is easier to maintain and extend, but that's just my\n> > personal preference.\n> \n> Adding device_add/del support to device-crash-test is certainly an\n> option. The problem is that nobody runs it by default, so this won't\n> help to avoid that new problems are being committed to the repository.\n> \n> I think we really should have a test for \"make check\", too. So would my\n> test be acceptable if I'd rewrite it to use QMP instead (I don't think I\n> could do the full list that Markus mentioned, but at least a basic test\n> via QMP as a start)?\n\nWe can run device-crash-test on \"make check\", we just need to\nchoose what's the subset of tests we want to run (because testing\nall machine+device+target combinations would take too long).\n\nBut while device-crash-test doesn't support hotplug, I think your\ntest code would be good too.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=ehabkost@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xsBpG1qpKz9s82\n\tfor ;\n\tWed, 13 Sep 2017 03:37:57 +1000 (AEST)","from localhost ([::1]:37819 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1drp8D-00038A-T6\n\tfor incoming@patchwork.ozlabs.org; Tue, 12 Sep 2017 13:37:53 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:55426)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drp7t-00037y-Vp\n\tfor qemu-devel@nongnu.org; Tue, 12 Sep 2017 13:37:35 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1drp7p-0003cu-2Z\n\tfor qemu-devel@nongnu.org; Tue, 12 Sep 2017 13:37:34 -0400","from mx1.redhat.com ([209.132.183.28]:51734)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1drp7o-0003c9-QA; Tue, 12 Sep 2017 13:37:29 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 9B0D57A;\n\tTue, 12 Sep 2017 17:37:27 +0000 (UTC)","from localhost (ovpn-116-15.gru2.redhat.com [10.97.116.15])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id E315560C22;\n\tTue, 12 Sep 2017 17:37:24 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 9B0D57A","Date":"Tue, 12 Sep 2017 14:37:23 -0300","From":"Eduardo Habkost ","To":"Thomas Huth ","Message-ID":"<20170912173723.GA7570@localhost.localdomain>","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>\n\t<87mv68e2i3.fsf@dusky.pond.sub.org>\n\t<20170909204133.GH7570@localhost.localdomain>\n\t<825b29f3-7857-558e-9092-855c9f75463e@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<825b29f3-7857-558e-9092-855c9f75463e@redhat.com>","X-Fnord":"you can see the fnord","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.29]);\n\tTue, 12 Sep 2017 17:37:27 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"qemu-devel@nongnu.org, Markus Armbruster , \"Dr. David\n\tAlan Gilbert\" , qemu-ppc@nongnu.org,\n\tIgor Mammedov , John Snow ,\n\tAndreas =?iso-8859-1?q?F=E4rber?= , Philippe\n\t=?iso-8859-1?q?Mathieu-Daud=E9?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1767587,"web_url":"http://patchwork.ozlabs.org/comment/1767587/","msgid":"<808c097f-a422-3aba-2e0d-d8360425f75b@redhat.com>","list_archive_url":null,"date":"2017-09-13T05:45:11","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":66152,"url":"http://patchwork.ozlabs.org/api/people/66152/","name":"Thomas Huth","email":"thuth@redhat.com"},"content":"On 12.09.2017 19:37, Eduardo Habkost wrote:\n> On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote:\n>> On 09.09.2017 22:41, Eduardo Habkost wrote:\n>>> On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote:\n>>>> Thomas Huth writes:\n>>>>\n>>>>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n>>>>>> * Markus Armbruster (armbru@redhat.com) wrote:\n>>>>>>> Thomas Huth writes:\n>>>>>>>\n>>>>>>>> People tend to forget to mark internal devices with \"user_creatable = false\n>>>>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n>>>>>>>> HMP monitor. So let's add a test to run through all devices and that tries\n>>>>>>>> to add them blindly (without arguments) to see whether this could crash the\n>>>>>>>> QEMU instance.\n>> [...]\n>>>>>>> * The device supports only cold plug with -device, not hot plug with\n>>>>>>> device_add.\n>>>>>\n>>>>> We've got Eduardo's scripts/device-crash-test script for that already,\n>>>>> so no need to cover that here.\n>>>>\n>>>> Point taken. So this test is really about hot plug / unplug. Suggest\n>>>> to clarify the commit message: s/add them blindly/hotplug and unplug\n>>>> them blindly/.\n>>>\n>>> We could extend device-crash-test to test device_add too, as it\n>>> already has extra code to deal with known crashes and testing\n>>> multiple machine-types. Also, any additional code we write to\n>>> ensure we add mandatory arguments or plug only to valid buses\n>>> would apply to both -device and device_add. I also think Python\n>>> test code is easier to maintain and extend, but that's just my\n>>> personal preference.\n>>\n>> Adding device_add/del support to device-crash-test is certainly an\n>> option. The problem is that nobody runs it by default, so this won't\n>> help to avoid that new problems are being committed to the repository.\n>>\n>> I think we really should have a test for \"make check\", too. So would my\n>> test be acceptable if I'd rewrite it to use QMP instead (I don't think I\n>> could do the full list that Markus mentioned, but at least a basic test\n>> via QMP as a start)?\n> \n> We can run device-crash-test on \"make check\", we just need to\n> choose what's the subset of tests we want to run (because testing\n> all machine+device+target combinations would take too long).\n\nMaybe we should just run it one time for every machine - and try to add\nall available devices at once?\n\n Thomas","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=thuth@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xsVy72lxlz9sPm\n\tfor ;\n\tWed, 13 Sep 2017 15:45:51 +1000 (AEST)","from localhost ([::1]:40265 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1ds0Uf-0001oR-He\n\tfor incoming@patchwork.ozlabs.org; Wed, 13 Sep 2017 01:45:49 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:34002)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1ds0UF-0001oI-3b\n\tfor qemu-devel@nongnu.org; Wed, 13 Sep 2017 01:45:24 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1ds0UB-0002vX-8H\n\tfor qemu-devel@nongnu.org; Wed, 13 Sep 2017 01:45:23 -0400","from mx1.redhat.com ([209.132.183.28]:41470)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1ds0UA-0002uM-Vg; Wed, 13 Sep 2017 01:45:19 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id D4E505D5EA;\n\tWed, 13 Sep 2017 05:45:16 +0000 (UTC)","from [10.36.116.123] (ovpn-116-123.ams2.redhat.com [10.36.116.123])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id E03BA5C6DC;\n\tWed, 13 Sep 2017 05:45:12 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com D4E505D5EA","To":"Eduardo Habkost ","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>\n\t<87mv68e2i3.fsf@dusky.pond.sub.org>\n\t<20170909204133.GH7570@localhost.localdomain>\n\t<825b29f3-7857-558e-9092-855c9f75463e@redhat.com>\n\t<20170912173723.GA7570@localhost.localdomain>","From":"Thomas Huth ","Message-ID":"<808c097f-a422-3aba-2e0d-d8360425f75b@redhat.com>","Date":"Wed, 13 Sep 2017 07:45:11 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170912173723.GA7570@localhost.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tWed, 13 Sep 2017 05:45:17 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"qemu-devel@nongnu.org, Markus Armbruster , \"Dr. David\n\tAlan Gilbert\" , qemu-ppc@nongnu.org,\n\tIgor Mammedov , \tJohn Snow ,\n\t=?utf-8?q?Andreas_F=C3=A4rber?= ,\n\t=?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1769489,"web_url":"http://patchwork.ozlabs.org/comment/1769489/","msgid":"<20170915221802.GA10621@localhost.localdomain>","list_archive_url":null,"date":"2017-09-15T22:18:02","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":195,"url":"http://patchwork.ozlabs.org/api/people/195/","name":"Eduardo Habkost","email":"ehabkost@redhat.com"},"content":"On Wed, Sep 13, 2017 at 07:45:11AM +0200, Thomas Huth wrote:\n> On 12.09.2017 19:37, Eduardo Habkost wrote:\n> > On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote:\n> >> On 09.09.2017 22:41, Eduardo Habkost wrote:\n> >>> On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote:\n> >>>> Thomas Huth writes:\n> >>>>\n> >>>>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n> >>>>>> * Markus Armbruster (armbru@redhat.com) wrote:\n> >>>>>>> Thomas Huth writes:\n> >>>>>>>\n> >>>>>>>> People tend to forget to mark internal devices with \"user_creatable = false\n> >>>>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n> >>>>>>>> HMP monitor. So let's add a test to run through all devices and that tries\n> >>>>>>>> to add them blindly (without arguments) to see whether this could crash the\n> >>>>>>>> QEMU instance.\n> >> [...]\n> >>>>>>> * The device supports only cold plug with -device, not hot plug with\n> >>>>>>> device_add.\n> >>>>>\n> >>>>> We've got Eduardo's scripts/device-crash-test script for that already,\n> >>>>> so no need to cover that here.\n> >>>>\n> >>>> Point taken. So this test is really about hot plug / unplug. Suggest\n> >>>> to clarify the commit message: s/add them blindly/hotplug and unplug\n> >>>> them blindly/.\n> >>>\n> >>> We could extend device-crash-test to test device_add too, as it\n> >>> already has extra code to deal with known crashes and testing\n> >>> multiple machine-types. Also, any additional code we write to\n> >>> ensure we add mandatory arguments or plug only to valid buses\n> >>> would apply to both -device and device_add. I also think Python\n> >>> test code is easier to maintain and extend, but that's just my\n> >>> personal preference.\n> >>\n> >> Adding device_add/del support to device-crash-test is certainly an\n> >> option. The problem is that nobody runs it by default, so this won't\n> >> help to avoid that new problems are being committed to the repository.\n> >>\n> >> I think we really should have a test for \"make check\", too. So would my\n> >> test be acceptable if I'd rewrite it to use QMP instead (I don't think I\n> >> could do the full list that Markus mentioned, but at least a basic test\n> >> via QMP as a start)?\n> > \n> > We can run device-crash-test on \"make check\", we just need to\n> > choose what's the subset of tests we want to run (because testing\n> > all machine+device+target combinations would take too long).\n> \n> Maybe we should just run it one time for every machine - and try to add\n> all available devices at once?\n\nYes, it makes sense. I will keep that in mind when trying to\nimplement device_add support on device-crash-test (but if anybody\nwants to volunteer to implement it, be my guest).","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=ehabkost@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xv8tp28qYz9s81\n\tfor ;\n\tSat, 16 Sep 2017 08:18:42 +1000 (AEST)","from localhost ([::1]:55184 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dsywa-0004cz-D9\n\tfor incoming@patchwork.ozlabs.org; Fri, 15 Sep 2017 18:18:40 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:37584)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dsyw9-0004bX-85\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 18:18:14 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dsyw6-0004sO-34\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 18:18:13 -0400","from mx1.redhat.com ([209.132.183.28]:36106)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dsyw5-0004s6-QZ; Fri, 15 Sep 2017 18:18:10 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 12ACD4E4CB;\n\tFri, 15 Sep 2017 22:18:07 +0000 (UTC)","from localhost (ovpn-116-24.gru2.redhat.com [10.97.116.24])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 7CBC561342;\n\tFri, 15 Sep 2017 22:18:04 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 12ACD4E4CB","Date":"Fri, 15 Sep 2017 19:18:02 -0300","From":"Eduardo Habkost ","To":"Thomas Huth ","Message-ID":"<20170915221802.GA10621@localhost.localdomain>","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>\n\t<87mv68e2i3.fsf@dusky.pond.sub.org>\n\t<20170909204133.GH7570@localhost.localdomain>\n\t<825b29f3-7857-558e-9092-855c9f75463e@redhat.com>\n\t<20170912173723.GA7570@localhost.localdomain>\n\t<808c097f-a422-3aba-2e0d-d8360425f75b@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<808c097f-a422-3aba-2e0d-d8360425f75b@redhat.com>","X-Fnord":"you can see the fnord","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.38]);\n\tFri, 15 Sep 2017 22:18:07 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"qemu-devel@nongnu.org, Markus Armbruster , \"Dr. David\n\tAlan Gilbert\" , qemu-ppc@nongnu.org,\n\tIgor Mammedov , John Snow ,\n\tAndreas =?iso-8859-1?q?F=E4rber?= , Philippe\n\t=?iso-8859-1?q?Mathieu-Daud=E9?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770660,"web_url":"http://patchwork.ozlabs.org/comment/1770660/","msgid":"<4b277dfb-3eba-c9c1-9028-15ac86a5c0e9@redhat.com>","list_archive_url":null,"date":"2017-09-19T05:25:48","subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","submitter":{"id":66152,"url":"http://patchwork.ozlabs.org/api/people/66152/","name":"Thomas Huth","email":"thuth@redhat.com"},"content":"On 16.09.2017 00:18, Eduardo Habkost wrote:\n> On Wed, Sep 13, 2017 at 07:45:11AM +0200, Thomas Huth wrote:\n>> On 12.09.2017 19:37, Eduardo Habkost wrote:\n>>> On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote:\n>>>> On 09.09.2017 22:41, Eduardo Habkost wrote:\n>>>>> On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote:\n>>>>>> Thomas Huth writes:\n>>>>>>\n>>>>>>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:\n>>>>>>>> * Markus Armbruster (armbru@redhat.com) wrote:\n>>>>>>>>> Thomas Huth writes:\n>>>>>>>>>\n>>>>>>>>>> People tend to forget to mark internal devices with \"user_creatable = false\n>>>>>>>>>> or hotpluggable = false, and these devices can crash QEMU if added via the\n>>>>>>>>>> HMP monitor. So let's add a test to run through all devices and that tries\n>>>>>>>>>> to add them blindly (without arguments) to see whether this could crash the\n>>>>>>>>>> QEMU instance.\n>>>> [...]\n>>>>>>>>> * The device supports only cold plug with -device, not hot plug with\n>>>>>>>>> device_add.\n>>>>>>>\n>>>>>>> We've got Eduardo's scripts/device-crash-test script for that already,\n>>>>>>> so no need to cover that here.\n>>>>>>\n>>>>>> Point taken. So this test is really about hot plug / unplug. Suggest\n>>>>>> to clarify the commit message: s/add them blindly/hotplug and unplug\n>>>>>> them blindly/.\n>>>>>\n>>>>> We could extend device-crash-test to test device_add too, as it\n>>>>> already has extra code to deal with known crashes and testing\n>>>>> multiple machine-types. Also, any additional code we write to\n>>>>> ensure we add mandatory arguments or plug only to valid buses\n>>>>> would apply to both -device and device_add. I also think Python\n>>>>> test code is easier to maintain and extend, but that's just my\n>>>>> personal preference.\n>>>>\n>>>> Adding device_add/del support to device-crash-test is certainly an\n>>>> option. The problem is that nobody runs it by default, so this won't\n>>>> help to avoid that new problems are being committed to the repository.\n>>>>\n>>>> I think we really should have a test for \"make check\", too. So would my\n>>>> test be acceptable if I'd rewrite it to use QMP instead (I don't think I\n>>>> could do the full list that Markus mentioned, but at least a basic test\n>>>> via QMP as a start)?\n>>>\n>>> We can run device-crash-test on \"make check\", we just need to\n>>> choose what's the subset of tests we want to run (because testing\n>>> all machine+device+target combinations would take too long).\n>>\n>> Maybe we should just run it one time for every machine - and try to add\n>> all available devices at once?\n> \n> Yes, it makes sense. I will keep that in mind when trying to\n> implement device_add support on device-crash-test (but if anybody\n> wants to volunteer to implement it, be my guest).\n\nNever mind, that was a unrealistic idea, since there are very likely\ndevices in the list that prevent QEMU from starting if a certain\nproperty is missing... so we likely won't catch any crashes with such a\ntest (unless we want to pedanticly maintain a blacklist of devices which\ncan not be used in that test ... you certainly got a very good start for\nthat in device-crash-test already, but I think the list will rather\nexplode if we want to get it usable for this idea?)\n\n Thomas","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=thuth@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxBF34hshz9s78\n\tfor ;\n\tTue, 19 Sep 2017 15:26:30 +1000 (AEST)","from localhost ([::1]:40112 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duB3D-0002hG-GN\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 01:26:27 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:39331)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duB2l-0002ff-5Y\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 01:26:00 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duB2h-00021o-6q\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 01:25:59 -0400","from mx1.redhat.com ([209.132.183.28]:50980)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duB2g-00021c-Tc; Tue, 19 Sep 2017 01:25:55 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id EAFC75D5EA;\n\tTue, 19 Sep 2017 05:25:53 +0000 (UTC)","from [10.36.116.49] (ovpn-116-49.ams2.redhat.com [10.36.116.49])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 1FAA35D97A;\n\tTue, 19 Sep 2017 05:25:49 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com EAFC75D5EA","To":"Eduardo Habkost ","References":"<1504605227-5124-1-git-send-email-thuth@redhat.com>\n\t<871snlwev0.fsf@dusky.pond.sub.org> <20170905164835.GF2112@work-vm>\n\t<6e03aa82-3663-c251-c8e1-ac2e7eb0e7e7@redhat.com>\n\t<87mv68e2i3.fsf@dusky.pond.sub.org>\n\t<20170909204133.GH7570@localhost.localdomain>\n\t<825b29f3-7857-558e-9092-855c9f75463e@redhat.com>\n\t<20170912173723.GA7570@localhost.localdomain>\n\t<808c097f-a422-3aba-2e0d-d8360425f75b@redhat.com>\n\t<20170915221802.GA10621@localhost.localdomain>","From":"Thomas Huth ","Message-ID":"<4b277dfb-3eba-c9c1-9028-15ac86a5c0e9@redhat.com>","Date":"Tue, 19 Sep 2017 07:25:48 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170915221802.GA10621@localhost.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tTue, 19 Sep 2017 05:25:54 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del\n\tHMP test","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"qemu-devel@nongnu.org, Markus Armbruster , \"Dr. David\n\tAlan Gilbert\" , qemu-ppc@nongnu.org,\n\tIgor Mammedov , \tJohn Snow ,\n\t=?utf-8?q?Andreas_F=C3=A4rber?= ,\n\t=?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]