From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YtGGb-00084x-J7 for qemu-devel@nongnu.org; Fri, 15 May 2015 10:07:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YtGGY-0004EF-5w for qemu-devel@nongnu.org; Fri, 15 May 2015 10:07:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41252) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YtGGX-0004Dh-Uy for qemu-devel@nongnu.org; Fri, 15 May 2015 10:07:06 -0400 Date: Fri, 15 May 2015 15:06:50 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150515140650.GA6733@work-vm> References: <1429031053-4454-1-git-send-email-dgilbert@redhat.com> <1429031053-4454-5-git-send-email-dgilbert@redhat.com> <20150515135044.GB15452@grmbl.mre> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150515135044.GB15452@grmbl.mre> Subject: Re: [Qemu-devel] [PATCH v6 04/47] Add qemu_get_counted_string to read a string prefixed by a count byte List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, "Dr. David Alan Gilbert (git)" , qemu-devel@nongnu.org, pbonzini@redhat.com, david@gibson.dropbear.id.au, yayanghy@cn.fujitsu.com * Amit Shah (amit.shah@redhat.com) wrote: > On (Tue) 14 Apr 2015 [18:03:30], Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > and use it in loadvm_state and ram_load. > > This patch is doing several things at once: > > - reducing size of a buffer from 257 to 256 (it's safe, but not > mentioned in the commit log) > > - adding an error return to one calling site (again not mentioned > here) > > > @@ -1145,13 +1145,10 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > > total_ram_bytes = addr; > > while (!ret && total_ram_bytes) { > > RAMBlock *block; > > - uint8_t len; > > char id[256]; > > ram_addr_t length; > > > > - len = qemu_get_byte(f); > > - qemu_get_buffer(f, (uint8_t *)id, len); > > - id[len] = 0; > > + qemu_get_counted_string(f, id); > > length = qemu_get_be64(f); > > > > QLIST_FOREACH_RCU(block, &ram_list.blocks, next) { > > - ... while not doing that for the other calling site. In fact we > really should check return value there too, isn't it? buf[len] is > set to 0, not buf[0] in case of an error, and ram_load could happily > start using string functions on the bogus data in id[]. > > Can you please split the patches up, or write a verbose commit > message? > > Also, I think you should just post these preparatory patches in a > separate series so they can be applied as they're good on their own. Yep; OK, I can split them out on an individual basis. > Postcopy patches themselves can come as another series, and that also > makes reviewing easier. > > Also: > > > + > > +/* > > + * Get a string whose length is determined by a single preceding byte > > + * A preallocated 256 byte buffer must be passed in. > > + * Returns: 0 on success and a 0 terminated string in the buffer > > + */ > > +int qemu_get_counted_string(QEMUFile *f, char buf[256]) > > +{ > > + unsigned int len = qemu_get_byte(f); > > + int res = qemu_get_buffer(f, (uint8_t *)buf, len); > > + > > + buf[len] = 0; > > + > > + return res != len; > > since you're returning bool, how about making this bool? Though I'd > like it if this was > > return res == len ? res : 0; > > BTW I'd like it if everything (return value, res, len) were all > unsigned. The operations are safe, but it sucks we use signed values > for counting things all over the place. Yes, I can do that (I intend some day to fix qemu_get_buffer and co to use size_t). Thanks, Dave > Thanks, > > Amit -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK