From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:50703)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <blauwirbel@gmail.com>) id 1QY3Is-0006Hw-55
	for qemu-devel@nongnu.org; Sat, 18 Jun 2011 17:43:43 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <blauwirbel@gmail.com>) id 1QY3Ir-0005hp-5M
	for qemu-devel@nongnu.org; Sat, 18 Jun 2011 17:43:42 -0400
Received: from mail-qw0-f45.google.com ([209.85.216.45]:54974)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <blauwirbel@gmail.com>) id 1QY3Ir-0005hk-1u
	for qemu-devel@nongnu.org; Sat, 18 Jun 2011 17:43:41 -0400
Received: by qwj8 with SMTP id 8so191424qwj.4
	for <qemu-devel@nongnu.org>; Sat, 18 Jun 2011 14:43:40 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <20110618034245.GA26613@amit-x200.redhat.com>
References: <20110616133849.02fb4bea@doriath>
	<20110617064736.GA18513@amit-x200.redhat.com>
	<20110617101644.5e275e53@doriath>
	<20110617160926.GA25648@amit-x200.redhat.com>
	<20110617150811.156acbae@doriath>
	<20110618034245.GA26613@amit-x200.redhat.com>
From: Blue Swirl <blauwirbel@gmail.com>
Date: Sun, 19 Jun 2011 00:43:20 +0300
Message-ID: <BANLkTinkCkpyP0JDWu5bhAT=XgFbo=ritQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Amit Shah <amit.shah@redhat.com>
Cc: mdroth@linux.vnet.ibm.com, Markus Armbruster <armbru@redhat.com>, qemu-devel <qemu-devel@nongnu.org>, Luiz Capitulino <lcapitulino@redhat.com>

On Sat, Jun 18, 2011 at 6:42 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote:
>
>> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!cpkt.value) {
>> > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error_report("virtio-seri=
al-bus: Guest failure in adding device %s\n",
>> > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 vser->bus.qbus.name);
>> > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
>> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error_report("virtio-seri=
al-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
>> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
>> >
>> > The line split should remain -- else it goes beyond 80 chars.
>>
>> It's already beyond 80 chars to me.
>
> I prefer to not break strings that get printed out -- makes it easier
> for greppers to find out the source of the string.

Please read CODING_STYLE and use scripts/checkpatch.pl before
submitting patches.

>> > > @@ -346,8 +339,13 @@ static void handle_control_message(VirtIOSerial=
 *vser, void *buf, size_t len)
>> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0QTAILQ_FOREACH(port, &vser->ports,=
 next) {
>> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0send_control_event(p=
ort, VIRTIO_CONSOLE_PORT_ADD, 1);
>> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>> > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
>> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
>> > > + =C2=A0 =C2=A0}
>> >
>> > Makes me think of one case (totally unrelated to what you found)where
>> > the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY
>> > messages.
>>
>> It will be handled just fine, no?
>
> We'll send out the VIRTIO_CONSOLE_PORT_ADD events for each port
> (again). =C2=A0That's the case now. =C2=A0No idea how the code might chan=
ge in
> the future and we could end up doing something else in addition which
> might be bad. =C2=A0Anyway, all this is for a buggy or a bad guest.
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Amit
>
>