| Message ID | 1447676789-7782-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
"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
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?
"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 :)
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; }
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(-)