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

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> On Tue, Nov 17, 2015 at 08:52:24AM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <address@hidden> writes:
>> 
>> > When we get an unexpected response, print out
>> > the original request.
>> > Helps debug protocol errors tremendously.
>> >
>> > Signed-off-by: Michael S. Tsirkin <address@hidden>
>> > ---
>> >
>> > 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: <address@hidden>
>> 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?

Okay.  Several ones in 2.6 is better than just one now :)