[PATCHv4,01/11] virtio-serial-bus: use correct lengths in control_out() message

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.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/virtio-serial-bus.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

On 03/15/2012 04:00 PM, 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.
>
> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru>
> ---
>   hw/virtio-serial-bus.c |    6 +++---
>   1 files changed, 3 insertions(+), 3 deletions(-)
>
> 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
> @@ -454,7 +454,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
>       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);
>           /*
> @@ -467,9 +467,9 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
>               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);

I don't understand what this is fixing.  copied = cur_len unless for some reason 
a full copy couldn't be done.

But you're assuming a full copy is done.  So I'm confused by what is being fixed 
here.

Regards,

Anthony Liguori

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

On 16.03.2012 20:12, Anthony Liguori wrote:
> On 03/15/2012 04:00 PM, 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.
>>
>> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru>
>> ---
>>   hw/virtio-serial-bus.c |    6 +++---
>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> 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
>> @@ -454,7 +454,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
>>       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);
>>           /*
>> @@ -467,9 +467,9 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
>>               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);
> 
> I don't understand what this is fixing.  copied = cur_len unless for some reason a full copy couldn't be done.
> 
> But you're assuming a full copy is done.  So I'm confused by what is being fixed here.

This is the same thing which the author of this code didn't
understand too, and the whole reason why I changed this code.

My answer:

http://thread.gmane.org/gmane.comp.emulators.qemu/140477/focus=140572

"We got one thing, we requested to copy another, and we handle
3rd which is something else.  While actually we are supposed
to get, request and handle the _same_, or else we're doomed."

In the same thread:

http://thread.gmane.org/gmane.comp.emulators.qemu/140477/focus=140572

"It is like doing, for a memcpy-like function:

 void *memdup(const void *src, size_t size) {
   char *dest = malloc(size+1);
   size_t copied = copybytes(dest, src, size+1);
   if (copied != size+1) {
      /* What?? */
   }
   return dest;
}

The only sane thing here, I think, is to drop 'copied',
to stop any possible confusion :)"

Thanks,

/mjt