From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40384) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQzQi-0006yh-8E for qemu-devel@nongnu.org; Fri, 21 Mar 2014 09:24:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQzQc-000705-4c for qemu-devel@nongnu.org; Fri, 21 Mar 2014 09:24:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1555) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQzQb-0006zc-SL for qemu-devel@nongnu.org; Fri, 21 Mar 2014 09:24:06 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2LDO4qZ018972 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 21 Mar 2014 09:24:05 -0400 From: Juan Quintela In-Reply-To: <1395320327-16613-1-git-send-email-dgilbert@redhat.com> (David Alan Gilbert's message of "Thu, 20 Mar 2014 12:58:47 +0000") References: <1395320327-16613-1-git-send-email-dgilbert@redhat.com> Date: Fri, 21 Mar 2014 14:24:02 +0100 Message-ID: <871txvit1p.fsf@elfo.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 1/1] Make qemu_peek_buffer loop until it gets it's data Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" Cc: qemu-devel@nongnu.org "Dr. David Alan Gilbert (git)" wrote: > From: "Dr. David Alan Gilbert" > > Make qemu_peek_buffer repatedly call fill_buffer until it gets > all the data it requires, or until there is an error. > > At the moment, qemu_peek_buffer will try one qemu_fill_buffer if there > isn't enough data waiting, however the kernel is entitled to return > just a few bytes, and still leave qemu_peek_buffer with less bytes > than it needed. I've seen this fail in a dev world, and I think it > could theoretically fail in the peeking of the subsection headers in > the current world. > > Ditto for qemu_peek_byte (which can only be affected due to it's > offset). > > Simplify qemu_get_buffer since it can now rely on qemu_peek_buffer to > loop. I think this one is wrong, will explain there. > Use size_t rather than int for size parameters, (and result for > those functions that never return -errno). Nice. > -int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size) > +size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size) > { > - int pending = size; > - int done = 0; > + size_t res; > > - while (pending > 0) { > - int res; > + res = qemu_peek_buffer(f, buf, size, 0); > > - res = qemu_peek_buffer(f, buf, pending, 0); > - if (res == 0) { > - return done; > - } > - qemu_file_skip(f, res); > - buf += res; > - pending -= res; > - done += res; > - } > - return done; > + qemu_file_skip(f, res); > + > + return res; I think this is "theoretical" (a.k.a. no user of this functionality). As this was coded, we could receive buffers bigger than IOBUF_SIZE, with your change, we can't. Just maintating the loop should fix this, right? > + while (index >= f->buf_size) { > + int received = qemu_fill_buffer(f); > + > + if (received <= 0) { here, I don't know really what to do. We just need one character, so the 1st call to qemu_fill_buffer() gives it to us, or we are already on problems. i.e. no need of the while() loop. On the other hand, having exactly the same code looks so nice. At some point I was thinking about making qemu_peek_byte() to use qemu_peek_buffer(), but I think that we used qemu_peek_byte() more to justify the overhead. I am talking from memory here. > diff --git a/vmstate.c b/vmstate.c > index d1f5eb0..b8e6e31 100644 > --- a/vmstate.c > +++ b/vmstate.c > @@ -170,7 +170,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, > } > size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2); > if (size != len) { > - return 0; > + return -EIO; > } > idstr[size] = 0; This was coded this way on purpose. If we don't have a valid buffer after the subsection identifier, just let the code continue to see if it wasn't a subsection at all. This colud be removed one tested that we don't allow subsections in the middle of a section, only in places where a section can appear. In general, very nice patch, and fixes the problem. Later, Juan.