Re: [Qemu-devel] [PATCH v2] vhost-user: print original request on error

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Nov 17, 2015 at 08:52:24AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > When we get an unexpected response, print out
> > the original request.
> > Helps debug protocol errors tremendously.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Changes from v1: add missing .
> >
> >  hw/virtio/vhost-user.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 3404b81..5bc6c45 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -121,8 +121,8 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
> >  
> >      r = qemu_chr_fe_read_all(chr, p, size);
> >      if (r != size) {
> > -        error_report("Failed to read msg header. Read %d instead of %d.", r,
> > -                size);
> > +        error_report("Failed to read msg header. Read %d instead of %d."
> > +                     " Original request %d.", r, size, msg->request);
> >          goto fail;
> >      }
> 
> Error message style nit: the error message proper (the thing emitted by
> error_report()) should be a phrase, and it should be short and to the
> point.  It can be followed by hints.  Compare:
> 
>     qemu-system-x86_64: Failed to read msg header. Read 11 instead of 12. Original request 1.
> 
> and
> 
>     qemu-system-x86_64: Failed to read msg header
>     Read 11 instead of 12 for request 1.
> 
> I prefer the latter.  The error message proper is short and to the
> point.  The hint adds information, which happens to be useful mainly to
> developers.  Sensible line lengths.
> 
> By the way, the error.h API supports this message + hints convention
> since commit 50b7b00.
> 
> See also
> Message-ID: <87oaf3jww7.fsf@blackfin.pond.sub.org>
> http://lists.gnu.org/archive/html/qemu-devel/2015-11/msg01662.html

Oh that's nice. There are more cases like this in vhost-user.
Let's rework them all past 2.5?

-- 
MST