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

[Michael Tokarev]

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<address@hidden>
>> ---
>>   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