Re: [Qemu-devel] [PATCH] spapr-vty: Fix bad assert() statement

[Paolo Bonzini]

----- Original Message -----
> From: "Thomas Huth" <address@hidden>
> To: "David Gibson" <address@hidden>
> Cc: address@hidden, "Alexander Graf" <address@hidden>, address@hidden, 
> address@hidden, "Gerd
> Hoffmann" <address@hidden>
> Sent: Friday, November 11, 2016 9:13:00 AM
> Subject: Re: [PATCH] spapr-vty: Fix bad assert() statement
> 
> On 11.11.2016 00:01, David Gibson wrote:
> > So, vastly increasing the buffer like this doesn't seem right - the
> > plain 16550 serial driver doesn't maintain a buffer bigger than its
> > small internal FIFO (32 characters IIRC) - or a single byte if the
> > FIFO is disabled.
> > 
> > I thought the other side of the char driver was supposed to call
> > can_receive() first and not deliver more bytes than we can handle -
> > hence the assert.
> 
> Right. The real bug seems to be on the front end side:
> 
> gd_vc_in() in ui/gtk.c calls qemu_chr_be_write() without checking the
> qemu_chr_be_can_write() first (which internally calls the can_receive()
> function of the backend).
> 
> So we likely need to fix gd_vc_in() to use qemu_chr_be_can_write()
> instead ... but I'm currently puzzled whether that can be done at all,
> since this seems to be a callback function - and we likely can not
> simply loop/yield here, can we?

You'd have to use a buffer in the GTK+ backend, in order to have
working cut and paste.  But at least using qemu_chr_be_can_write,
and limiting the size of the written data to the return value,
would fix the assertion.

> > That said, I think vty_can_receive() has a bug - looking at other
> > drivers, I think it's supposed to return the amount of buffer space
> > available, but we're just returning 0 or 1.  Although AFAICT that
> > should still work, just inefficiently.
> 
> Right, it should return the amount of free space instead.

Yes.

Paolo