From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44685) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7SHo-0007ng-83 for qemu-devel@nongnu.org; Tue, 13 Mar 2012 10:01:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S7SHi-0001N8-NV for qemu-devel@nongnu.org; Tue, 13 Mar 2012 10:01:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18782) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7SHi-0001Mo-FQ for qemu-devel@nongnu.org; Tue, 13 Mar 2012 10:01:06 -0400 Date: Tue, 13 Mar 2012 19:31:00 +0530 From: Amit Shah Message-ID: <20120313140100.GB7344@amit.redhat.com> References: <20120311135745.7866A65E0@gandalf.tls.msk.ru> <20120312085909.GB17840@amit.redhat.com> <4F5DC04E.8040509@msgid.tls.msk.ru> <20120312110633.GC17840@amit.redhat.com> <4F5DE187.80309@msgid.tls.msk.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4F5DE187.80309@msgid.tls.msk.ru> Subject: Re: [Qemu-devel] [PATCH] virtio-serial-bus: use correct lengths in control_out() message List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: qemu-devel@nongnu.org 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