On Wed, Dec 04, 2013 at 06:40:30PM +0100, Paolo Bonzini wrote:
> Il 04/12/2013 15:06, Stefan Hajnoczi ha scritto:
> > On Thu, Oct 10, 2013 at 05:07:18PM +0200, Paolo Bonzini wrote:
> >> @@ -298,30 +278,31 @@ static void handle_notify(EventNotifier *e)
> >> vring_disable_notification(s->vdev, &s->vring);
> >>
> >> for (;;) {
> >> - head = vring_pop(s->vdev, &s->vring, iov, end, &out_num,
> >> &in_num);
> >> - if (head < 0) {
> >> + ret = vring_pop(s->vdev, &s->vring, &elem);
> >> + if (ret < 0) {
> >> + assert(elem == NULL);
> >> break; /* no more requests */
> >> }
> >>
> >> - trace_virtio_blk_data_plane_process_request(s, out_num,
> >> in_num,
> >> - head);
> >> + trace_virtio_blk_data_plane_process_request(s, elem->out_num,
> >> + elem->in_num,
> >> elem->index);
> >>
> >> - if (process_request(&s->ioqueue, iov, out_num, in_num, head)
> >> < 0) {
> >> + if (process_request(&s->ioqueue, elem) < 0) {
> >> vring_set_broken(&s->vring);
> >> + vring_push(&s->vring, elem, 0);
> >
> > If we give up on the vring I don't think we should push the element
> > back. It may cause the guest to panic.
> >
> > I guess what we really need here is to unmap scatter-gather buffers and
> > delete elem.
>
> That's what already happens actually. vring_push has
>
>
> + g_slice_free(VirtQueueElement, elem);
> +
> /* Don't touch vring if a fatal error occurred */
> if (vring->broken) {
> return;
>
> in this patch and
>
> + for (i = 0; i < elem->out_num; i++) {
> + vring_unmap(elem->out_sg[i].iov_base, false);
> + }
> +
> + for (i = 0; i < elem->in_num; i++) {
> + vring_unmap(elem->in_sg[i].iov_base, true);
> + }
>
> g_slice_free(VirtQueueElement, elem);
>
> in the next one.
>
> Though I admit vring_push isn't such a great name and API. I can add
> instead a vring_free_element function. Do you think vring_push should
> call it, or should the caller do that?
I think vring_push() should free the VirtQueueElement.
We just need to expose vring_free_element() so that handle_notify() can
call it without pushing bogus buffers back to the guest.
Stefan
