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!
