From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 479F6203B85BA for ; Wed, 16 May 2018 00:22:17 -0700 (PDT) Date: Wed, 16 May 2018 09:22:08 +0200 From: Igor Mammedov Subject: Re: [Qemu-devel] [PATCH 3/3] nvdimm: platform capabilities command line option Message-ID: <20180516092208.4461c748@redhat.com> In-Reply-To: <20180515230351.GA741@linux.intel.com> References: <20180427215314.23168-1-ross.zwisler@linux.intel.com> <20180427215314.23168-3-ross.zwisler@linux.intel.com> <20180510152848.0f99ea07@redhat.com> <20180515230351.GA741@linux.intel.com> MIME-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: Ross Zwisler Cc: Eduardo Habkost , linux-nvdimm , "Michael S . Tsirkin" , qemu-devel@nongnu.org, Stefan Hajnoczi List-ID: On Tue, 15 May 2018 17:03:51 -0600 Ross Zwisler wrote: > On Thu, May 10, 2018 at 03:28:48PM +0200, Igor Mammedov wrote: [...] > > > Also an extra patch to for make check that will test setting 'cap' > > would be nice (an extra testcase in tests/bios-tables-test.c) > > Hmm...I've been looking at this, and it doesn't look like there is any > verification around a lot of the ACPI tables (NFIT, SRAT, etc). as far as I recall NFIT and SRAT are verified against expected template (limited but at least something) Following commits can serve as an example: e0e5c85 test/acpi-test-data: add ACPI tables for dimmpxm test adae91c tests/bios-tables-test: add test cases for DIMM proximity > I've verified my patch by interacting with a guest with various settings - is > this good enough, or do you really want me to test the value (which I think > would just be "do I get out what I put in at the command line") via the unit > test infrastructure? It would be better to add test especially for a new code. The reason for it is to catch regressions down the road, it also makes easier for maintainer to review/test series. > Thank you for the review. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44093) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fIqlQ-0006Rz-LE for qemu-devel@nongnu.org; Wed, 16 May 2018 03:22:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fIqlN-0002Kx-I7 for qemu-devel@nongnu.org; Wed, 16 May 2018 03:22:20 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:39270 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fIqlN-0002KN-DF for qemu-devel@nongnu.org; Wed, 16 May 2018 03:22:17 -0400 Date: Wed, 16 May 2018 09:22:08 +0200 From: Igor Mammedov Message-ID: <20180516092208.4461c748@redhat.com> In-Reply-To: <20180515230351.GA741@linux.intel.com> References: <20180427215314.23168-1-ross.zwisler@linux.intel.com> <20180427215314.23168-3-ross.zwisler@linux.intel.com> <20180510152848.0f99ea07@redhat.com> <20180515230351.GA741@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] nvdimm: platform capabilities command line option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ross Zwisler Cc: Haozhong Zhang , "Michael S . Tsirkin" , Stefan Hajnoczi , Eduardo Habkost , qemu-devel@nongnu.org, linux-nvdimm On Tue, 15 May 2018 17:03:51 -0600 Ross Zwisler wrote: > On Thu, May 10, 2018 at 03:28:48PM +0200, Igor Mammedov wrote: [...] > > > Also an extra patch to for make check that will test setting 'cap' > > would be nice (an extra testcase in tests/bios-tables-test.c) > > Hmm...I've been looking at this, and it doesn't look like there is any > verification around a lot of the ACPI tables (NFIT, SRAT, etc). as far as I recall NFIT and SRAT are verified against expected template (limited but at least something) Following commits can serve as an example: e0e5c85 test/acpi-test-data: add ACPI tables for dimmpxm test adae91c tests/bios-tables-test: add test cases for DIMM proximity > I've verified my patch by interacting with a guest with various settings - is > this good enough, or do you really want me to test the value (which I think > would just be "do I get out what I put in at the command line") via the unit > test infrastructure? It would be better to add test especially for a new code. The reason for it is to catch regressions down the road, it also makes easier for maintainer to review/test series. > Thank you for the review.