From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39026) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YtG16-0001tf-0w for qemu-devel@nongnu.org; Fri, 15 May 2015 09:51:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YtG12-0006zA-Qs for qemu-devel@nongnu.org; Fri, 15 May 2015 09:51:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56837) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YtG12-0006z4-JP for qemu-devel@nongnu.org; Fri, 15 May 2015 09:51:04 -0400 Date: Fri, 15 May 2015 19:20:44 +0530 From: Amit Shah Message-ID: <20150515135044.GB15452@grmbl.mre> References: <1429031053-4454-1-git-send-email-dgilbert@redhat.com> <1429031053-4454-5-git-send-email-dgilbert@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429031053-4454-5-git-send-email-dgilbert@redhat.com> 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: "Dr. David Alan Gilbert (git)" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, david@gibson.dropbear.id.au, yayanghy@cn.fujitsu.com 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. 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. Thanks, Amit