From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55897) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WR0bb-0001Af-7X for qemu-devel@nongnu.org; Fri, 21 Mar 2014 10:39:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WR0bU-0006eN-Kb for qemu-devel@nongnu.org; Fri, 21 Mar 2014 10:39:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51081) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WR0bU-0006e1-Ct for qemu-devel@nongnu.org; Fri, 21 Mar 2014 10:39:24 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2LEdN3w026723 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 21 Mar 2014 10:39:23 -0400 Date: Fri, 21 Mar 2014 14:39:19 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20140321143919.GA8476@work-vm> References: <1395320327-16613-1-git-send-email-dgilbert@redhat.com> <871txvit1p.fsf@elfo.mitica> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <871txvit1p.fsf@elfo.mitica> Subject: Re: [Qemu-devel] [PATCH 1/1] Make qemu_peek_buffer loop until it gets it's data List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: "Dr. David Alan Gilbert (git)" , qemu-devel@nongnu.org * Juan Quintela (quintela@redhat.com) wrote: > "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? Ah, actually that is a good point (I've got a feeling one of my other worlds relies on that); yes, I'll put the loop back and fix it all to be size_t. > > + 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. The problem is that peek_byte takes an offset, so while qemu_fill_buffer will get us a byte, we actually need it to get us all the bytes upto the offset, and that's not guaranteed from one call. > 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. Thanks, I'll rework and get a V2 up later. Dave > > Later, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK