| Message ID | 20170628190047.26159-26-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/28/2017 09:00 PM, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > When we receive a GET_VRING_BASE message set enable = false > to stop any new received packets modifying the ring. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> Maxime > --- > contrib/libvhost-user/libvhost-user.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c > index ceddeac74f..d37052b7b0 100644 > --- a/contrib/libvhost-user/libvhost-user.c > +++ b/contrib/libvhost-user/libvhost-user.c > @@ -652,6 +652,7 @@ vu_get_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg) > vmsg->size = sizeof(vmsg->payload.state); > > dev->vq[index].started = false; > + dev->vq[index].enable = false; > if (dev->iface->queue_set_started) { > dev->iface->queue_set_started(dev, index, false); > } >
On Wed, Jun 28, 2017 at 08:00:43PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > When we receive a GET_VRING_BASE message set enable = false > to stop any new received packets modifying the ring. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> I think I already reviewed a similar patch. Spec says: Client must only process each ring when it is started. IMHO the real fix is to fix client to check the started flag before processing the ring. > --- > contrib/libvhost-user/libvhost-user.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c > index ceddeac74f..d37052b7b0 100644 > --- a/contrib/libvhost-user/libvhost-user.c > +++ b/contrib/libvhost-user/libvhost-user.c > @@ -652,6 +652,7 @@ vu_get_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg) > vmsg->size = sizeof(vmsg->payload.state); > > dev->vq[index].started = false; > + dev->vq[index].enable = false; > if (dev->iface->queue_set_started) { > dev->iface->queue_set_started(dev, index, false); > } > -- > 2.13.0
* Michael S. Tsirkin (mst@redhat.com) wrote: > On Wed, Jun 28, 2017 at 08:00:43PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > When we receive a GET_VRING_BASE message set enable = false > > to stop any new received packets modifying the ring. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > I think I already reviewed a similar patch. Yes you replied to my off-list mail; I hadn't got around to fixing it yet. > Spec says: > Client must only process each ring when it is started. but in that reply you said the spec said Client must only pass data between the ring and the backend, when the ring is enabled. So does the spec say 'started' or 'enabled' (Pointer to the spec?) > IMHO the real fix is to fix client to check the started > flag before processing the ring. Yep I can do that. I was curious however whether it was specified as 'started' or 'enabled' or both. Dave > > --- > > contrib/libvhost-user/libvhost-user.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c > > index ceddeac74f..d37052b7b0 100644 > > --- a/contrib/libvhost-user/libvhost-user.c > > +++ b/contrib/libvhost-user/libvhost-user.c > > @@ -652,6 +652,7 @@ vu_get_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg) > > vmsg->size = sizeof(vmsg->payload.state); > > > > dev->vq[index].started = false; > > + dev->vq[index].enable = false; > > if (dev->iface->queue_set_started) { > > dev->iface->queue_set_started(dev, index, false); > > } > > -- > > 2.13.0 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, Jul 05, 2017 at 06:16:17PM +0100, Dr. David Alan Gilbert wrote: > * Michael S. Tsirkin (mst@redhat.com) wrote: > > On Wed, Jun 28, 2017 at 08:00:43PM +0100, Dr. David Alan Gilbert (git) wrote: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > When we receive a GET_VRING_BASE message set enable = false > > > to stop any new received packets modifying the ring. > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > I think I already reviewed a similar patch. > > Yes you replied to my off-list mail; I hadn't got > around to fixing it yet. > > > Spec says: > > Client must only process each ring when it is started. > > but in that reply you said the spec said > > Client must only pass data between the ring and the > backend, when the ring is enabled. > > So does the spec say 'started' or 'enabled' Both. Ring processing is limited by ring being started. Passing actual data - by ring also being enabled. With ring started but disabled you drop all packets. > (Pointer to the spec?) It's part of QEMU source: docs/interop/vhost-user.txt > > IMHO the real fix is to fix client to check the started > > flag before processing the ring. > > Yep I can do that. I was curious however whether it was > specified as 'started' or 'enabled' or both. > > Dave > > > > --- > > > contrib/libvhost-user/libvhost-user.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c > > > index ceddeac74f..d37052b7b0 100644 > > > --- a/contrib/libvhost-user/libvhost-user.c > > > +++ b/contrib/libvhost-user/libvhost-user.c > > > @@ -652,6 +652,7 @@ vu_get_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg) > > > vmsg->size = sizeof(vmsg->payload.state); > > > > > > dev->vq[index].started = false; > > > + dev->vq[index].enable = false; > > > if (dev->iface->queue_set_started) { > > > dev->iface->queue_set_started(dev, index, false); > > > } > > > -- > > > 2.13.0 > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Michael S. Tsirkin (mst@redhat.com) wrote: > On Wed, Jun 28, 2017 at 08:00:43PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > When we receive a GET_VRING_BASE message set enable = false > > to stop any new received packets modifying the ring. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > I think I already reviewed a similar patch. > Spec says: > Client must only process each ring when it is started. > > IMHO the real fix is to fix client to check the started > flag before processing the ring. Done, I added a vu_queue_started to match vu_queue_enabled and then used it. Dave > > --- > > contrib/libvhost-user/libvhost-user.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c > > index ceddeac74f..d37052b7b0 100644 > > --- a/contrib/libvhost-user/libvhost-user.c > > +++ b/contrib/libvhost-user/libvhost-user.c > > @@ -652,6 +652,7 @@ vu_get_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg) > > vmsg->size = sizeof(vmsg->payload.state); > > > > dev->vq[index].started = false; > > + dev->vq[index].enable = false; > > if (dev->iface->queue_set_started) { > > dev->iface->queue_set_started(dev, index, false); > > } > > -- > > 2.13.0 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c index ceddeac74f..d37052b7b0 100644 --- a/contrib/libvhost-user/libvhost-user.c +++ b/contrib/libvhost-user/libvhost-user.c @@ -652,6 +652,7 @@ vu_get_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg) vmsg->size = sizeof(vmsg->payload.state); dev->vq[index].started = false; + dev->vq[index].enable = false; if (dev->iface->queue_set_started) { dev->iface->queue_set_started(dev, index, false); }