| Message ID | 1466503372-28334-9-git-send-email-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 21, 2016 at 12:02:36PM +0200, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Return read errors (not sure why those were ignored) > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> why bother? So callers can just ignore them in turn? > --- > hw/virtio/vhost-user.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index e51df27..819481d 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -221,7 +221,7 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, > if (shmfd) { > msg.size = 0; > if (vhost_user_read(dev, &msg) < 0) { > - return 0; > + return -1; > } > > if (msg.request != VHOST_USER_SET_LOG_BASE) { > @@ -373,7 +373,7 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev, > } > > if (vhost_user_read(dev, &msg) < 0) { > - return 0; > + return -1; > } > > if (msg.request != VHOST_USER_GET_VRING_BASE) { > @@ -474,7 +474,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64) > } > > if (vhost_user_read(dev, &msg) < 0) { > - return 0; > + return -1; > } > > if (msg.request != request) { > -- > 2.7.4
Hi ----- Original Message ----- > On Tue, Jun 21, 2016 at 12:02:36PM +0200, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Return read errors (not sure why those were ignored) > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > why bother? So callers can just ignore them in turn? The callers do not always ignore errors, fortunately (see vhost_user_init() for ex). That at least prevents flooding terminal/monitor with multiple errors and helps finding out the origin of the error. > > > --- > > hw/virtio/vhost-user.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index e51df27..819481d 100644 > > --- a/hw/virtio/vhost-user.c > > +++ b/hw/virtio/vhost-user.c > > @@ -221,7 +221,7 @@ static int vhost_user_set_log_base(struct vhost_dev > > *dev, uint64_t base, > > if (shmfd) { > > msg.size = 0; > > if (vhost_user_read(dev, &msg) < 0) { > > - return 0; > > + return -1; > > } > > > > if (msg.request != VHOST_USER_SET_LOG_BASE) { > > @@ -373,7 +373,7 @@ static int vhost_user_get_vring_base(struct vhost_dev > > *dev, > > } > > > > if (vhost_user_read(dev, &msg) < 0) { > > - return 0; > > + return -1; > > } > > > > if (msg.request != VHOST_USER_GET_VRING_BASE) { > > @@ -474,7 +474,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev, > > int request, uint64_t *u64) > > } > > > > if (vhost_user_read(dev, &msg) < 0) { > > - return 0; > > + return -1; > > } > > > > if (msg.request != request) { > > -- > > 2.7.4 >
On Thu, Jun 23, 2016 at 05:14:04AM -0400, Marc-André Lureau wrote: > Hi > > ----- Original Message ----- > > On Tue, Jun 21, 2016 at 12:02:36PM +0200, marcandre.lureau@redhat.com wrote: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > Return read errors (not sure why those were ignored) > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > why bother? So callers can just ignore them in turn? > > The callers do not always ignore errors, fortunately (see > vhost_user_init() for ex). But that doesn't call set log base, right? > That at least prevents flooding > terminal/monitor with multiple errors and helps finding out the origin > of the error. > > > > > > --- > > > hw/virtio/vhost-user.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > > index e51df27..819481d 100644 > > > --- a/hw/virtio/vhost-user.c > > > +++ b/hw/virtio/vhost-user.c > > > @@ -221,7 +221,7 @@ static int vhost_user_set_log_base(struct vhost_dev > > > *dev, uint64_t base, > > > if (shmfd) { > > > msg.size = 0; > > > if (vhost_user_read(dev, &msg) < 0) { > > > - return 0; > > > + return -1; > > > } > > > > > > if (msg.request != VHOST_USER_SET_LOG_BASE) { > > > @@ -373,7 +373,7 @@ static int vhost_user_get_vring_base(struct vhost_dev > > > *dev, > > > } > > > > > > if (vhost_user_read(dev, &msg) < 0) { > > > - return 0; > > > + return -1; > > > } > > > > > > if (msg.request != VHOST_USER_GET_VRING_BASE) { > > > @@ -474,7 +474,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev, > > > int request, uint64_t *u64) > > > } > > > > > > if (vhost_user_read(dev, &msg) < 0) { > > > - return 0; > > > + return -1; > > > } > > > > > > if (msg.request != request) { > > > -- > > > 2.7.4 > >
Hi On Thu, Jun 23, 2016 at 7:03 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > why bother? So callers can just ignore them in turn? >> >> The callers do not always ignore errors, fortunately (see >> vhost_user_init() for ex). > > But that doesn't call set log base, right? No, what is the point you are making?
On Fri, Jun 24, 2016 at 02:46:28PM +0200, Marc-André Lureau wrote: > Hi > > On Thu, Jun 23, 2016 at 7:03 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > why bother? So callers can just ignore them in turn? > >> > >> The callers do not always ignore errors, fortunately (see > >> vhost_user_init() for ex). > > > > But that doesn't call set log base, right? > > No, what is the point you are making? Why does vhost_user_set_log_base need to return error? If backend is not there to handle this message, then it is not changing memory so it's ok to ignore the error. Same logic applies to many other messages. > > -- > Marc-André Lureau
Hi On Mon, Jul 4, 2016 at 5:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > Why does vhost_user_set_log_base need to return error? > If backend is not there to handle this message, > then it is not changing memory so it's ok to ignore the error. How do you know it's not changing the memory? Furthermore, if the migration happened, it's because backend claim VHOST_F_LOG_ALL, thus it should really not fail > Same logic applies to many other messages. Pretty much all messages, the error can't be ignored, or operations will just fail silentely randomly. I don't understand why vhost-user io error can be ignored. Also it's quite inconsistent the way the code is today, vhost_user_write() returns an error that is mostly ignored, while vhost_user_read() is checked. Why having an error later when you can report it earlier? I fail to understand the rationale of this error handling.
On Mon, Jul 04, 2016 at 11:56:56PM +0200, Marc-André Lureau wrote: > Hi > > On Mon, Jul 4, 2016 at 5:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > Why does vhost_user_set_log_base need to return error? > > If backend is not there to handle this message, > > then it is not changing memory so it's ok to ignore the error. > > How do you know it's not changing the memory? either it closed socket intentionally or it exited and kernel cleaned up. > Furthermore, if the migration happened, it's because backend claim > VHOST_F_LOG_ALL, thus it should really not fail I don't see why - could you explain pls? > > Same logic applies to many other messages. > > Pretty much all messages, the error can't be ignored, or operations > will just fail silentely randomly. I don't understand why vhost-user > io error can be ignored. Also it's quite inconsistent the way the code > is today, vhost_user_write() returns an error that is mostly ignored, > while vhost_user_read() is checked. Why having an error later when you > can report it earlier? I fail to understand the rationale of this > error handling. It's historical. the way I see it, most errors due to disconnect can be ignored except maybe for the initial feature negotiation which is needed so we know what to tell guest. Errors due to e.g. buffer being full should cause an assert as it's an internal qemu error. > > -- > Marc-André Lureau
Hi On Tue, Jul 5, 2016 at 12:35 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Jul 04, 2016 at 11:56:56PM +0200, Marc-André Lureau wrote: >> Hi >> >> On Mon, Jul 4, 2016 at 5:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > Why does vhost_user_set_log_base need to return error? >> > If backend is not there to handle this message, >> > then it is not changing memory so it's ok to ignore the error. >> >> How do you know it's not changing the memory? > > either it closed socket intentionally or it exited > and kernel cleaned up. And if it closed intentionally during migration, we want to catch this as a bug since it may still modify the memory >> Furthermore, if the migration happened, it's because backend claim >> VHOST_F_LOG_ALL, thus it should really not fail > > I don't see why - could you explain pls? If the backend claims migration support, it shouldn't have bad migration behaviour such as closing the vhost-user socket. >> > Same logic applies to many other messages. >> >> Pretty much all messages, the error can't be ignored, or operations >> will just fail silentely randomly. I don't understand why vhost-user >> io error can be ignored. Also it's quite inconsistent the way the code >> is today, vhost_user_write() returns an error that is mostly ignored, >> while vhost_user_read() is checked. Why having an error later when you >> can report it earlier? I fail to understand the rationale of this >> error handling. > > It's historical. the way I see it, most errors due to disconnect > can be ignored except > maybe for the initial feature negotiation which is needed > so we know what to tell guest. The way I see it is that errors should not be ignored because it makes it harder to track what is going on. > Errors due to e.g. buffer being full should cause an assert > as it's an internal qemu error. I don't see why qemu would be responsible for say, a suspended backend. > > >> >> -- >> Marc-André Lureau
On Tue, Jul 05, 2016 at 11:18:38AM +0200, Marc-André Lureau wrote: > Hi > > On Tue, Jul 5, 2016 at 12:35 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Jul 04, 2016 at 11:56:56PM +0200, Marc-André Lureau wrote: > >> Hi > >> > >> On Mon, Jul 4, 2016 at 5:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > Why does vhost_user_set_log_base need to return error? > >> > If backend is not there to handle this message, > >> > then it is not changing memory so it's ok to ignore the error. > >> > >> How do you know it's not changing the memory? > > > > either it closed socket intentionally or it exited > > and kernel cleaned up. > > And if it closed intentionally during migration, we want to catch this > as a bug since it may still modify the memory You can't prevent backend bugs I think. > >> Furthermore, if the migration happened, it's because backend claim > >> VHOST_F_LOG_ALL, thus it should really not fail > > > > I don't see why - could you explain pls? > > If the backend claims migration support, it shouldn't have bad > migration behaviour such as closing the vhost-user socket. But I don't see why it's bad. If it's not modifying memory then it does not need to log any changes. > >> > Same logic applies to many other messages. > >> > >> Pretty much all messages, the error can't be ignored, or operations > >> will just fail silentely randomly. I don't understand why vhost-user > >> io error can be ignored. Also it's quite inconsistent the way the code > >> is today, vhost_user_write() returns an error that is mostly ignored, > >> while vhost_user_read() is checked. Why having an error later when you > >> can report it earlier? I fail to understand the rationale of this > >> error handling. > > > > It's historical. the way I see it, most errors due to disconnect > > can be ignored except > > maybe for the initial feature negotiation which is needed > > so we know what to tell guest. > > The way I see it is that errors should not be ignored because it makes > it harder to track what is going on. > > > Errors due to e.g. buffer being full should cause an assert > > as it's an internal qemu error. > > I don't see why qemu would be responsible for say, a suspended backend. Protocol limits # of messages in flight so it should not trigger even if backend is stuck. > > > > > >> > >> -- > >> Marc-André Lureau > > > > -- > Marc-André Lureau
Hi On Tue, Jul 5, 2016 at 1:12 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Jul 05, 2016 at 11:18:38AM +0200, Marc-André Lureau wrote: >> Hi >> >> On Tue, Jul 5, 2016 at 12:35 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Mon, Jul 04, 2016 at 11:56:56PM +0200, Marc-André Lureau wrote: >> >> Hi >> >> >> >> On Mon, Jul 4, 2016 at 5:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> > Why does vhost_user_set_log_base need to return error? >> >> > If backend is not there to handle this message, >> >> > then it is not changing memory so it's ok to ignore the error. >> >> >> >> How do you know it's not changing the memory? >> > >> > either it closed socket intentionally or it exited >> > and kernel cleaned up. >> >> And if it closed intentionally during migration, we want to catch this >> as a bug since it may still modify the memory > > You can't prevent backend bugs I think. Right, but it's best to provide an error when you can detect backend bugs. >> >> Furthermore, if the migration happened, it's because backend claim >> >> VHOST_F_LOG_ALL, thus it should really not fail >> > >> > I don't see why - could you explain pls? >> >> If the backend claims migration support, it shouldn't have bad >> migration behaviour such as closing the vhost-user socket. > > But I don't see why it's bad. If it's not modifying memory then > it does not need to log any changes. "if it's not modifying memory"... I fail to understand why some code path check error code, and some don't. Ignoring error and running further may lead to wrong assumptions and later issues. I also fail to understand why providing more useful error messages is bad. I feel quite strongly about having more consistent error checking in vhost-user, I don't get why you don't.
On Wed, Jul 6, 2016 at 3:40 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Tue, Jul 5, 2016 at 1:12 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> On Tue, Jul 05, 2016 at 11:18:38AM +0200, Marc-André Lureau wrote: >>> Hi >>> >>> On Tue, Jul 5, 2016 at 12:35 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> > On Mon, Jul 04, 2016 at 11:56:56PM +0200, Marc-André Lureau wrote: >>> >> Hi >>> >> >>> >> On Mon, Jul 4, 2016 at 5:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >> > Why does vhost_user_set_log_base need to return error? >>> >> > If backend is not there to handle this message, >>> >> > then it is not changing memory so it's ok to ignore the error. >>> >> >>> >> How do you know it's not changing the memory? >>> > >>> > either it closed socket intentionally or it exited >>> > and kernel cleaned up. >>> >>> And if it closed intentionally during migration, we want to catch this >>> as a bug since it may still modify the memory >> >> You can't prevent backend bugs I think. > > Right, but it's best to provide an error when you can detect backend bugs. > >>> >> Furthermore, if the migration happened, it's because backend claim >>> >> VHOST_F_LOG_ALL, thus it should really not fail >>> > >>> > I don't see why - could you explain pls? >>> >>> If the backend claims migration support, it shouldn't have bad >>> migration behaviour such as closing the vhost-user socket. >> >> But I don't see why it's bad. If it's not modifying memory then >> it does not need to log any changes. > > "if it's not modifying memory"... > > I fail to understand why some code path check error code, and some > don't. Ignoring error and running further may lead to wrong > assumptions and later issues. I also fail to understand why providing > more useful error messages is bad. I feel quite strongly about having > more consistent error checking in vhost-user, I don't get why you > don't. > Fwiw, Gonglei came up with the same patch in "vhost-user: fix unreasonable return value when vhost-user read failed" and he gave extra reasons for it that are hard to deny.
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index e51df27..819481d 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -221,7 +221,7 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, if (shmfd) { msg.size = 0; if (vhost_user_read(dev, &msg) < 0) { - return 0; + return -1; } if (msg.request != VHOST_USER_SET_LOG_BASE) { @@ -373,7 +373,7 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev, } if (vhost_user_read(dev, &msg) < 0) { - return 0; + return -1; } if (msg.request != VHOST_USER_GET_VRING_BASE) { @@ -474,7 +474,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64) } if (vhost_user_read(dev, &msg) < 0) { - return 0; + return -1; } if (msg.request != request) {