From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41029) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S716B-00088X-2n for qemu-devel@nongnu.org; Mon, 12 Mar 2012 04:59:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S7164-0008L6-Pt for qemu-devel@nongnu.org; Mon, 12 Mar 2012 04:59:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44851) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7164-0008Kh-I5 for qemu-devel@nongnu.org; Mon, 12 Mar 2012 04:59:16 -0400 Date: Mon, 12 Mar 2012 14:29:09 +0530 From: Amit Shah Message-ID: <20120312085909.GB17840@amit.redhat.com> References: <20120311135745.7866A65E0@gandalf.tls.msk.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120311135745.7866A65E0@gandalf.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 (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? One comment below. > 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. > - 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); > } Thanks, Amit