Re: [Qemu-devel] [PATCH] virtio-serial-bus: use correct lengths in control_out() message

[Amit Shah]

On (Sun) 11 Mar 2012 [17:52:59], Michael Tokarev wrote:
> In case of more than one control message, the code will use
> size of the largest message so far for all subsequent messages,
> instead of using size of current one.  Fix it.

Makes sense.  How did you detect this?  Any reproducible test-case?

One comment below.

> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index e22940e..abe48ec 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -451,28 +451,28 @@ static void control_out(VirtIODevice *vdev, VirtQueue 
> *vq)
>  
>      vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
>  
>      len = 0;
>      buf = NULL;
>      while (virtqueue_pop(vq, &elem)) {
> -        size_t cur_len, copied;
> +        size_t cur_len;
>  
>          cur_len = iov_size(elem.out_sg, elem.out_num);
>          /*
>           * Allocate a new buf only if we didn't have one previously or
>           * if the size of the buf differs
>           */
>          if (cur_len > len) {
>              g_free(buf);
>  
>              buf = g_malloc(cur_len);
>              len = cur_len;
>          }
> -        copied = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, len);
> +        iov_to_buf(elem.out_sg, elem.out_num, buf, 0, cur_len);

Why drop 'copied'?  I don't think we have had a situation where copied
can be less than cur_len, and in any case we don't do anything special
as a recovery mechanism, but a warning message or an abort in case
copied != cur_len should work, I think.

> -        handle_control_message(vser, buf, copied);
> +        handle_control_message(vser, buf, cur_len);
>          virtqueue_push(vq, &elem, 0);
>      }
>      g_free(buf);
>      virtio_notify(vdev, vq);
>  }

Thanks,

                Amit