Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Thu, 8 Sep 2016 18:19:27 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:
> > On Thu, 8 Sep 2016 18:00:28 +0300
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >   
> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:  
> > > > On Thu, 8 Sep 2016 10:59:26 +0200
> > > > Cornelia Huck <address@hidden> wrote:
> > > >   
> > > > > On Wed, 07 Sep 2016 19:19:24 +0200
> > > > > Greg Kurz <address@hidden> wrote:
> > > > >   
> > > > > > Calling assert() really makes sense when hitting a genuine bug, 
> > > > > > which calls
> > > > > > for a fix in QEMU. However, when something goes wrong because the 
> > > > > > guest
> > > > > > sends a malformed message, it is better to write down a more 
> > > > > > meaningul
> > > > > > error message and exit.
> > > > > > 
> > > > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > > > ---
> > > > > >  hw/9pfs/virtio-9p-device.c |   20 ++++++++++++++++++--
> > > > > >  1 file changed, 18 insertions(+), 2 deletions(-)    
> > > > > 
> > > > > While this is an improvement over the current state, I don't think the
> > > > > guest should be able to kill qemu just by doing something stupid.
> > > > >   
> > > > 
> > > > Hi Connie,
> > > > 
> > > > I'm glad you're pointing this out... this was also my impression, but
> > > > since there are a bunch of sanity checks in the virtio code that cause
> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
> > > > stand up :)  
> > > 
> > > It's true that it's broken in many places but we should just
> > > fix them all.
> > > 
> > > 
> > > A separate question is how to log such hardware/guest bugs generally.
> > > People already complained about disk filling up because of us printing
> > > errors on each such bug.  Maybe print each message only N times, and
> > > then set a flag to skip the log until management tells us to restart
> > > logging again.  
> > 
> > I'd expect to get the message just once per device if we set the device
> > to broken (unless the guess continuously resets it again...)  
> 
> Which it can do, so we should limit that anyway.
> 
> > Do we have
> > a generic print/log ratelimit infrastructure in qemu?  
> 
> There are actually two kinds of errors
> host side ones and ones triggered by guests.
> 
> We should distinguish between them API-wise, then
> we will be able to limit the logging of those
> that guest can trigger.
> 

FWIW it makes sense to use error_report() if QEMU exits. If it continues
execution, this means we're expecting the guest or the host to do something
to fix the error condition. This requires QEMU to emit an event of some
sort, but not necessarily to log an error message in a file. I guess this
depends if QEMU is run by some tooling, or by a human.