From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39288) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S735Q-0008Pd-Fl for qemu-devel@nongnu.org; Mon, 12 Mar 2012 07:07:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S735L-0000Im-Gd for qemu-devel@nongnu.org; Mon, 12 Mar 2012 07:06:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59596) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S735L-0000Ic-8d for qemu-devel@nongnu.org; Mon, 12 Mar 2012 07:06:39 -0400 Date: Mon, 12 Mar 2012 16:36:33 +0530 From: Amit Shah Message-ID: <20120312110633.GC17840@amit.redhat.com> References: <20120311135745.7866A65E0@gandalf.tls.msk.ru> <20120312085909.GB17840@amit.redhat.com> <4F5DC04E.8040509@msgid.tls.msk.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4F5DC04E.8040509@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 [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?) > >> 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. Heh. > And > please note that even you, the author, don't understand > what it is doing :) Of course, I can't claim to remember everything, I sometimes don't even remember stuff *while* coding it. However, I do understand this part of the code, what I meant above was iov_to_buf() could fail or copy lesser than what was asked of it. 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 I think the patch is correct > still ;) No doubt about that. I never said otherwise. I just feel we shouldn't ignore return values. Amit