From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41520) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlsaS-0001el-NA for qemu-devel@nongnu.org; Tue, 13 Oct 2015 01:57:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZlsaN-0005v6-Np for qemu-devel@nongnu.org; Tue, 13 Oct 2015 01:57:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49574) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlsaN-0005v2-Ic for qemu-devel@nongnu.org; Tue, 13 Oct 2015 01:57:19 -0400 Date: Tue, 13 Oct 2015 08:57:12 +0300 From: "Michael S. Tsirkin" Message-ID: <20151013084304-mutt-send-email-mst@redhat.com> References: <1444535584-18220-1-git-send-email-guangrong.xiao@linux.intel.com> <20151012144113-mutt-send-email-mst@redhat.com> <561C96CC.40702@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <561C96CC.40702@linux.intel.com> Subject: Re: [Qemu-devel] [PATCH v3 00/32] implement vNVDIMM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: ehabkost@redhat.com, kvm@vger.kernel.org, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On Tue, Oct 13, 2015 at 01:29:48PM +0800, Xiao Guangrong wrote: > > > On 10/12/2015 07:55 PM, Michael S. Tsirkin wrote: > >On Sun, Oct 11, 2015 at 11:52:32AM +0800, Xiao Guangrong wrote: > >>Changelog in v3: > >>There is huge change in this version, thank Igor, Stefan, Paolo, Eduardo, > >>Michael for their valuable comments, the patchset finally gets better shape. > > > >Thanks! > >This needs some changes in coding style, and more comments, to > >make it easier to maintain going forward. > > Thanks for your review, Michael. I have learned lots of thing from > your comments. > > > > >High level comments - I didn't point out all instances, > >please go over code and locate them yourself. > >I focused on acpi code in this review. > > Okay, will do. > > > > > - fix coding style violations, prefix eveything with nvdimm_ etc > > Actually i did not pay attention on naming the stuff which is only internally > used. Thank you for pointing it out and will fix it in next version. > > > - in apci code, avoid manual memory management/complex pointer math > > I am not very good at ACPI ASL/AML, could you please more detail? It's about C. For example: Foo *foo = acpi_data_push(table, sizeof *foo); Bar *foo = acpi_data_push(table, sizeof *bar); is pretty obviously safe, and it doesn't require you to do any calculations. char *buf = acpi_data_push(table, sizeof *foo + sizeof *bar); is worse, now you need: Bar *bar = (Bar *)(buf + sizeof *foo); which will corrupt memory if you get the size wrong in push. > > - comments are needed to document apis & explain what's going on > > - constants need comments too, refer to text that > > can be looked up in acpi spec verbatim > > Indeed, will document carefully.