On 12.03.2012 12:59, Amit Shah wrote:
> 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?
There's no test-case, and no detection, just reading the code.
Actually, I think, there's no bug here, but a very, well,
difficult to read code.
> One comment below.
and the answer is below, too.
>> 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.
In this case, copied was _always_ == cur_len. That's why there's
actually no bug. See:
cur_len = iov_size(elem.out_sg, elem.out_num);
len = max(cur_len, buflen) <= "roughly"
copied = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, len);
iov_to_buf() will stop copying when it reaches end of buf
(which is "len" bytes long) or end of iov, which is cur_len
bytes long. Obviously in all cases it will be cur_len.
But it is obvious only when you write it one near another
and _think_. And the reason for this confusion is the
introduction of this `copied' variable, which shouldn't
be there in the first place.
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 :)
>> - 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);
>> }
Please belive me, I realized that the original code is
actually right only after re-reading your reply. And
please note that even you, the author, don't understand
what it is doing :) So I think the patch is correct
still ;)
Thanks!
/mjt
