From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47335) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzN0o-0002lb-BN for qemu-devel@nongnu.org; Wed, 26 Oct 2016 08:08:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzN0j-0005CY-Be for qemu-devel@nongnu.org; Wed, 26 Oct 2016 08:08:54 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:37787) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bzN0j-0005Bv-1R for qemu-devel@nongnu.org; Wed, 26 Oct 2016 08:08:49 -0400 Received: from pps.filterd (m0098404.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u9QC8iqq146757 for ; Wed, 26 Oct 2016 08:08:47 -0400 Received: from e06smtp10.uk.ibm.com (e06smtp10.uk.ibm.com [195.75.94.106]) by mx0a-001b2d01.pphosted.com with ESMTP id 26apkka0yg-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 26 Oct 2016 08:08:47 -0400 Received: from localhost by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 26 Oct 2016 13:08:42 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 8F07317D8056 for ; Wed, 26 Oct 2016 13:10:54 +0100 (BST) Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u9QC8dsX30474334 for ; Wed, 26 Oct 2016 12:08:39 GMT Received: from d06av08.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u9QC8dMC029504 for ; Wed, 26 Oct 2016 06:08:39 -0600 References: <20161021143741.8597-1-pasic@linux.vnet.ibm.com> <20161021143741.8597-4-pasic@linux.vnet.ibm.com> <20161025101346.GB2034@work-vm> From: Halil Pasic Date: Wed, 26 Oct 2016 14:08:39 +0200 MIME-Version: 1.0 In-Reply-To: <20161025101346.GB2034@work-vm> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Message-Id: <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: "Dr. David Alan Gilbert" Cc: Amit Shah , Guenther Hutzl , qemu-devel@nongnu.org, Juan Quintela 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? > 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. 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