[{"id":1760666,"web_url":"http://patchwork.ozlabs.org/comment/1760666/","msgid":"<62fe261c-09d4-e4c2-fb88-b8c05b700e56@redhat.com>","list_archive_url":null,"date":"2017-08-31T06:10:26","subject":"Re: [Qemu-devel] [PATCH 3/9] s390x/css: be more consistent if\n\tbroken beyond repair","submitter":{"id":66152,"url":"http://patchwork.ozlabs.org/api/people/66152/","name":"Thomas Huth","email":"thuth@redhat.com"},"content":"On 30.08.2017 18:36, Halil Pasic wrote:\n> If we detect that the internally manged state of the subchannel\n> is broken beyond repair while in do_subchannel_work in case of\n> virtual we just abort the operation and pretend all went well,\n> while in case of pass-through we honor the situation with -ENODEV\n> which results in cc 3 for the instruction whose handler triggered\n> the call.\n> \n> Let's be consistent on this and do the -ENODEV also for virtual\n> subchannels.\n> \n> Signed-off-by: Halil Pasic \n> Acked-by: Pierre Morel\n> ---\n> hw/s390x/css.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n> index 0822538cde..bc47bf5b20 100644\n> --- a/hw/s390x/css.c\n> +++ b/hw/s390x/css.c\n> @@ -1078,7 +1078,7 @@ int do_subchannel_work_virtual(SubchDev *sch)\n> sch_handle_start_func_virtual(sch);\n> } else {\n> /* Cannot happen. */\n> - return 0;\n> + return -ENODEV;\n> }\n> css_inject_io_interrupt(sch);\n> return 0;\n\nFirst, I think ENODEV is not really a good choice here since there\ncertainly was a device. So maybe use EINVAL or EBADR or something else\ninstead?\n\nSecond, while return an error code is certainly better than returning 0,\nI think most errors will still go unnoticed here, since most callers of\ndo_subchannel_work() seem to ignore the return value ... so I wonder\nwhether we rather want to have another way to recognize this condition.\nIf the comment is right and this really can not happen, I think you\nshould use an g_assert_notreached() here instead. Otherwise the comment\nshould be changed and it's maybe a good idea to use a\nqemu_log_mask(LOG_GUEST_ERROR, \"subchannel in bad state bla bla...\") here.\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 3xjX795K81z9sMN\n\tfor ;\n\tThu, 31 Aug 2017 16:11:00 +1000 (AEST)","from localhost ([::1]:54084 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dnIgr-0007Vo-L0\n\tfor incoming@patchwork.ozlabs.org; Thu, 31 Aug 2017 02:10:57 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47224)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dnIgW-0007Vj-0R\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 02:10:36 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dnIgR-0005cY-QY\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 02:10:35 -0400","from mx1.redhat.com ([209.132.183.28]:60618)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dnIgR-0005c2-KK\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 02:10:31 -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 53D0F76A13;\n\tThu, 31 Aug 2017 06:10:30 +0000 (UTC)","from [10.36.116.27] (ovpn-116-27.ams2.redhat.com [10.36.116.27])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 997ACA74A0;\n\tThu, 31 Aug 2017 06:10:28 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 53D0F76A13","To":"Halil Pasic , Cornelia Huck ","References":"<20170830163609.50260-1-pasic@linux.vnet.ibm.com>\n\t<20170830163609.50260-4-pasic@linux.vnet.ibm.com>","From":"Thomas Huth ","Message-ID":"<62fe261c-09d4-e4c2-fb88-b8c05b700e56@redhat.com>","Date":"Thu, 31 Aug 2017 08:10:26 +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":"<20170830163609.50260-4-pasic@linux.vnet.ibm.com>","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.27]);\n\tThu, 31 Aug 2017 06:10:30 +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] [PATCH 3/9] s390x/css: be more consistent if\n\tbroken beyond repair","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":"Dong Jia Shi ,\n\tPierre Morel , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1760716,"web_url":"http://patchwork.ozlabs.org/comment/1760716/","msgid":"","list_archive_url":null,"date":"2017-08-31T07:44:55","subject":"Re: [Qemu-devel] [PATCH 3/9] s390x/css: be more consistent if\n\tbroken beyond repair","submitter":{"id":66152,"url":"http://patchwork.ozlabs.org/api/people/66152/","name":"Thomas Huth","email":"thuth@redhat.com"},"content":"On 31.08.2017 08:10, Thomas Huth wrote:\n> On 30.08.2017 18:36, Halil Pasic wrote:\n>> If we detect that the internally manged state of the subchannel\n>> is broken beyond repair while in do_subchannel_work in case of\n>> virtual we just abort the operation and pretend all went well,\n>> while in case of pass-through we honor the situation with -ENODEV\n>> which results in cc 3 for the instruction whose handler triggered\n>> the call.\n>>\n>> Let's be consistent on this and do the -ENODEV also for virtual\n>> subchannels.\n>>\n>> Signed-off-by: Halil Pasic \n>> Acked-by: Pierre Morel\n>> ---\n>> hw/s390x/css.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n>> index 0822538cde..bc47bf5b20 100644\n>> --- a/hw/s390x/css.c\n>> +++ b/hw/s390x/css.c\n>> @@ -1078,7 +1078,7 @@ int do_subchannel_work_virtual(SubchDev *sch)\n>> sch_handle_start_func_virtual(sch);\n>> } else {\n>> /* Cannot happen. */\n>> - return 0;\n>> + return -ENODEV;\n>> }\n>> css_inject_io_interrupt(sch);\n>> return 0;\n> \n> First, I think ENODEV is not really a good choice here since there\n> certainly was a device. So maybe use EINVAL or EBADR or something else\n> instead?\n>\n> Second, while return an error code is certainly better than returning 0,\n> I think most errors will still go unnoticed here, since most callers of\n> do_subchannel_work() seem to ignore the return value ... so I wonder\n> whether we rather want to have another way to recognize this condition.\n> If the comment is right and this really can not happen, I think you\n> should use an g_assert_notreached() here instead. Otherwise the comment\n> should be changed and it's maybe a good idea to use a\n> qemu_log_mask(LOG_GUEST_ERROR, \"subchannel in bad state bla bla...\") here.\n\nOK, after reading through patch 4/9 I think I've got the basic idea now\n... you'll eventually set sch->iret.cc = 3 here instead, so the exact\nerror code does not really matter here.\nBut still - if it \"Cannot happen\", maybe an assert() or an additional\nqemu_log would be appropriate?\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-mx01.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx01.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 3xjZDG6z4Nz9sQl\n\tfor ;\n\tThu, 31 Aug 2017 17:45:34 +1000 (AEST)","from localhost ([::1]:54349 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dnKAO-0007jh-Uh\n\tfor incoming@patchwork.ozlabs.org; Thu, 31 Aug 2017 03:45:33 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35316)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dnK9v-0007dm-SO\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 03:45:09 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dnK9r-0005o0-KO\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 03:45:03 -0400","from mx1.redhat.com ([209.132.183.28]:58010)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dnK9r-0005ng-Dg\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 03:44:59 -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 2E43581DEE;\n\tThu, 31 Aug 2017 07:44:58 +0000 (UTC)","from [10.36.116.27] (ovpn-116-27.ams2.redhat.com [10.36.116.27])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id E0C5E91B27;\n\tThu, 31 Aug 2017 07:44:56 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 2E43581DEE","From":"Thomas Huth ","To":"Halil Pasic , Cornelia Huck ","References":"<20170830163609.50260-1-pasic@linux.vnet.ibm.com>\n\t<20170830163609.50260-4-pasic@linux.vnet.ibm.com>\n\t<62fe261c-09d4-e4c2-fb88-b8c05b700e56@redhat.com>","Message-ID":"","Date":"Thu, 31 Aug 2017 09:44:55 +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":"<62fe261c-09d4-e4c2-fb88-b8c05b700e56@redhat.com>","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.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.25]);\n\tThu, 31 Aug 2017 07:44:58 +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] [PATCH 3/9] s390x/css: be more consistent if\n\tbroken beyond repair","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":"Dong Jia Shi ,\n\tPierre Morel , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1760783,"web_url":"http://patchwork.ozlabs.org/comment/1760783/","msgid":"<20170831113340.7b437fa7.cohuck@redhat.com>","list_archive_url":null,"date":"2017-08-31T09:33:40","subject":"Re: [Qemu-devel] [PATCH 3/9] s390x/css: be more consistent if\n\tbroken beyond repair","submitter":{"id":71914,"url":"http://patchwork.ozlabs.org/api/people/71914/","name":"Cornelia Huck","email":"cohuck@redhat.com"},"content":"On Wed, 30 Aug 2017 18:36:03 +0200\nHalil Pasic wrote:\n\n> If we detect that the internally manged state of the subchannel\n> is broken beyond repair while in do_subchannel_work in case of\n> virtual we just abort the operation and pretend all went well,\n> while in case of pass-through we honor the situation with -ENODEV\n> which results in cc 3 for the instruction whose handler triggered\n> the call.\n> \n> Let's be consistent on this and do the -ENODEV also for virtual\n> subchannels.\n> \n> Signed-off-by: Halil Pasic \n> Acked-by: Pierre Morel\n> ---\n> hw/s390x/css.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n> index 0822538cde..bc47bf5b20 100644\n> --- a/hw/s390x/css.c\n> +++ b/hw/s390x/css.c\n> @@ -1078,7 +1078,7 @@ int do_subchannel_work_virtual(SubchDev *sch)\n> sch_handle_start_func_virtual(sch);\n> } else {\n> /* Cannot happen. */\n> - return 0;\n> + return -ENODEV;\n\nNo, this _really_ cannot happen. fctl is a three-bit field, which means\nthat one of the branches above must have executed. fctl cannot be 0, as\nany caller of do_subchannel_work() either sets a bit there or, in the\ncase of rsch, checks for a bit already set. This is an internal error,\nso an assert seems more suitable here. [We might need to keep the\nreturn to keep mingw happy.]\n\n> }\n> css_inject_io_interrupt(sch);\n> return 0;","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=cohuck@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 3xjcdf2V6zz9s7F\n\tfor ;\n\tThu, 31 Aug 2017 19:34:14 +1000 (AEST)","from localhost ([::1]:54658 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dnLrY-0007oq-FQ\n\tfor incoming@patchwork.ozlabs.org; Thu, 31 Aug 2017 05:34:12 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56471)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dnLr9-0007ml-Is\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 05:33:48 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dnLr6-0006qm-Fe\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 05:33:47 -0400","from mx1.redhat.com ([209.132.183.28]:39664)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dnLr6-0006q1-8c\n\tfor qemu-devel@nongnu.org; Thu, 31 Aug 2017 05:33:44 -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 18A827E43E;\n\tThu, 31 Aug 2017 09:33:43 +0000 (UTC)","from gondolin (dhcp-192-215.str.redhat.com [10.33.192.215])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 53BBB92B93;\n\tThu, 31 Aug 2017 09:33:42 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 18A827E43E","Date":"Thu, 31 Aug 2017 11:33:40 +0200","From":"Cornelia Huck ","To":"Halil Pasic ","Message-ID":"<20170831113340.7b437fa7.cohuck@redhat.com>","In-Reply-To":"<20170830163609.50260-4-pasic@linux.vnet.ibm.com>","References":"<20170830163609.50260-1-pasic@linux.vnet.ibm.com>\n\t<20170830163609.50260-4-pasic@linux.vnet.ibm.com>","Organization":"Red Hat GmbH","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","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.27]);\n\tThu, 31 Aug 2017 09:33:43 +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] [PATCH 3/9] s390x/css: be more consistent if\n\tbroken beyond repair","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":"Dong Jia Shi ,\n\tPierre Morel , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]