From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52436) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzNLq-0003vk-PV for qemu-devel@nongnu.org; Wed, 26 Oct 2016 08:30:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzNLn-0006ir-Kt for qemu-devel@nongnu.org; Wed, 26 Oct 2016 08:30:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55380) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bzNLn-0006hu-Fx for qemu-devel@nongnu.org; Wed, 26 Oct 2016 08:30:35 -0400 Date: Wed, 26 Oct 2016 13:30:31 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161026123030.GE2029@work-vm> References: <20161021143741.8597-1-pasic@linux.vnet.ibm.com> <20161021143741.8597-4-pasic@linux.vnet.ibm.com> <20161025101346.GB2034@work-vm> <9ffa224d-11b2-5d54-ffb7-9ae9409262b7@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9ffa224d-11b2-5d54-ffb7-9ae9409262b7@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH 3/4] migration/vmstate: fix array of pointers to struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Halil Pasic Cc: Amit Shah , Guenther Hutzl , qemu-devel@nongnu.org, Juan Quintela * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 10/25/2016 12:13 PM, Dr. David Alan Gilbert wrote: > [..] > >> for (i = 0; i < n_elems; i++) { > >> - void *addr = base_addr + size * i; > >> + void *curr_elem = first_elem + size * i; > > > > This diff is quite confusing because a lot of it involves the > > rename of 'addr' to 'curr_elem' at the same time as you change > > the structure. It would be better to split the renaming into > > a separate patch to make this clearer or just leave the name > > the same. > > > > You are absolutely right this is a Frankestein of a cleanup > patch and the actual patch. I will split the cleanup out. > > The patch is also conceptually based on my remove .start patch > it's just that I wanted to make the RFC independent so it can > be tested more easily. > > [..] > >> @@ -720,6 +722,27 @@ const VMStateInfo vmstate_info_uint64 = { > >> .put = put_uint64, > >> }; > >> > >> +static int get_nullptr(QEMUFile *f, void *pv, size_t size) > >> +{ > >> + int8_t tmp; > >> + qemu_get_s8s(f, &tmp); > >> + assert(tmp == 0); > > > > There's no need for the assert there, just return -EINVAL, > > then we'll get a clear error. > > Will do. > > > Also, '0' is a bad value to use just as a check - if the field is wrong then > > 0 often appears in the next byte anyway; > > > > Absolutely right. How about -1? -1 is OK (although you could use any character - e.g. N (for Null)). > > However, I'm not sure it's worth having the info_nullptr; > > if we just leave out the whole info_nullptr then you should still > > be protected by the section footer, although this may be > > able to give a better error. > > > > IMHO this can (in some cases) guard against the case we have the > same number of elements on source and on target, but at different > positions (e.g. {ptr0, NULL, NULL} and {NULL, ptr1, NULL}. The footers > should not be able to detect this. Yes, you're right it does give that extra protection and is worth it. Dave > Thank you very much for the thorough review! I will wait a bit > to see if more discussion happens, and then send out a non RFC > version with the issues addressed. > > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK