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

[Michael Tokarev <mjt@tls.msk.ru>]

On 12.03.2012 15:06, Amit Shah wrote:
> On (Mon) 12 Mar 2012 [13:22:22], Michael Tokarev wrote:
>> 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.
> 
> Do you mean this code is difficult to read, or in general?  Any ideas
> to make it simpler (or at least details on what's difficult?)

Just difficult to understand, and just this particular (very minor!)
place.

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.

[]
>       It's a memcpy() right now, it
> could change to something else.  Ignoring return values, esp. in copy
> functions, is not good style, even if you know it can't fail.

So that's the reason why the return value should be void, and
that the code should always request as many bytes as it actually
needs, and there must be some assert()s to ensure we're not
outside of something.

>>  So I think the patch is correct
>> still ;)
> 
> No doubt about that.  I never said otherwise.  I just feel we
> shouldn't ignore return values.

By _not_ ignoring return value in something like that is not far
away from checking if 1 is still equal to 1 after each instruction :)
I want to change this interface a bit, to be more obvious, to
stop all similar discussions and doubts.  It will handle the
requested amount and will abort() internally if it can't, so
we can stop bothering ignoring the return value, provided that
we actually request what we _want_ it to do, not what we have.
In this particular case, the 'size' argument of iov_from_buf()
should be 'bytes' or 'len', -- actual amount of bytes we need
to process, not size of the buffer we have in our disposal.

(For the iov_* family, we've another set, qemu_iovec_*, and
also qemu_sendv() &Co, each of which have different and not
obvious at all interface :)

Thanks!