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

[Amit Shah]

On (Mon) 12 Mar 2012 [15:44:07], Michael Tokarev wrote:
> 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.

No doom anywhere, but thanks for the details.

> []
> >       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.

Can you make this patch a part of that series, then?

> (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 :)

It's a feature to be consistent within one codebase :)

                Amit