From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58576) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8a7f-00080N-F4 for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:35:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S8a7G-0000kR-I2 for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:35:23 -0400 Received: from isrv.corpit.ru ([86.62.121.231]:54665) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8a7G-0000kK-AM for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:34:58 -0400 Message-ID: <4F636BAE.3050702@msgid.tls.msk.ru> Date: Fri, 16 Mar 2012 20:34:54 +0400 From: Michael Tokarev MIME-Version: 1.0 References: <1331845217-21705-1-git-send-email-mjt@msgid.tls.msk.ru> <1331845217-21705-2-git-send-email-mjt@msgid.tls.msk.ru> <4F636664.5090505@redhat.com> In-Reply-To: <4F636664.5090505@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv4 01/11] virtio-serial-bus: use correct lengths in control_out() message List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Paolo Bonzini , qemu-devel@nongnu.org On 16.03.2012 20:12, Anthony Liguori wrote: > On 03/15/2012 04:00 PM, 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. >> >> Signed-off-by: Michael Tokarev >> --- >> hw/virtio-serial-bus.c | 6 +++--- >> 1 files changed, 3 insertions(+), 3 deletions(-) >> >> 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 >> @@ -454,7 +454,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq) >> 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); >> /* >> @@ -467,9 +467,9 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq) >> 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); > > I don't understand what this is fixing. copied = cur_len unless for some reason a full copy couldn't be done. > > But you're assuming a full copy is done. So I'm confused by what is being fixed here. This is the same thing which the author of this code didn't understand too, and the whole reason why I changed this code. My answer: http://thread.gmane.org/gmane.comp.emulators.qemu/140477/focus=140572 "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." In the same thread: http://thread.gmane.org/gmane.comp.emulators.qemu/140477/focus=140572 "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 :)" Thanks, /mjt