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

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

[Michael S. Tsirkin]

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;
     }
 
-- 
MST

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

[Markus Armbruster]

"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

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

[Michael S. Tsirkin]

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

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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?

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