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