From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmQ1h-00077B-RA for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:30:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VmQ1b-00032X-QP for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:30:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41036) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmQ1b-00032T-IZ for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:30:35 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rATFUHc3011454 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 29 Nov 2013 10:30:17 -0500 Message-ID: <5298B304.1050504@redhat.com> Date: Fri, 29 Nov 2013 16:30:12 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1385662155-15212-1-git-send-email-lersek@redhat.com> <1385662155-15212-5-git-send-email-lersek@redhat.com> <87eh5zl2pq.fsf@blackfin.pond.sub.org> In-Reply-To: <87eh5zl2pq.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/4] i440fx-test: verify firmware under 4G and 1M, both -bios and -pflash List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On 11/29/13 15:09, Markus Armbruster wrote: > Laszlo Ersek writes: >> + /* check below 4G */ >> + buf = g_malloc0(FW_SIZE); >> + memread(0x100000000ULL - FW_SIZE, buf, FW_SIZE); > > Zero-fill immediately followed by read. Suggest g_malloc(). This was intentional. Please see the preexistent use of memread() in this file, in verify_area(). > >> + for (i = 0; i < FW_SIZE; ++i) { >> + g_assert_cmphex(buf[i], ==, (char unsigned)i); > > (unsigned char), please. > >> + } >> + >> + /* check in ISA space too */ >> + memset(buf, 0, FW_SIZE); >> + isa_bios_size = ISA_BIOS_MAXSZ < FW_SIZE ? ISA_BIOS_MAXSZ : FW_SIZE; >> + memread(0x100000 - isa_bios_size, buf, isa_bios_size); > > Zero-fill immediately followed by read. Suggest to drop memset(). Same as above. memread() is unable to report errors. Some C library functions also require you to set errno to zero first, then call the function, then check errno, because some of the return values are overlapped by success and failure returns. For memread() there's no distinction in return value at all. > >> + for (i = 0; i < isa_bios_size; ++i) { >> + g_assert_cmphex(buf[i], ==, >> + (char unsigned)((FW_SIZE - isa_bios_size) + i)); > > Again. > >> + } >> + >> + g_free(buf); >> + qtest_end(); >> +} >> + >> +static void add_firmware_test(const char *testpath, >> + void (*setup_fixture)(FirmwareTestFixture *f, >> + gconstpointer test_data)) >> +{ >> + g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture, >> + test_i440fx_firmware, NULL); >> +} >> + >> +static void request_bios(FirmwareTestFixture *fixture, >> + gconstpointer user_data) >> +{ >> + fixture->is_bios = true; >> +} >> + >> +static void request_pflash(FirmwareTestFixture *fixture, >> + gconstpointer user_data) >> +{ >> + fixture->is_bios = false; >> +} >> + > > I'm not sure fixtures are justified here. Perhaps having the test > function's test data argument point to the flag would be simpler. I gave a lot of thought to this. Fixtures are needed. See the explanation in patch#2. In more detail here: the gtest framework operates in two distinct phases. First, you set up all of your test cases in *declarative* style. That's what all the g_test_add_*() invocations do. You need to describe everything in advance. Then, you call g_test_run(), and it runs your declarative script in one atomic sequence. You cannot change stuff *between* tests. If I want to run the same test function twice, but with different data, I have the following choices: - Allocate and initialize two distinct memory areas. The lifetimes of both of these will be identical, and they will both live during the entire test series. I configure the first test case with the address of the first area, and the 2nd case with the address of the 2nd area. - Keep one global (shared) area, and let the tests read and write it as they are executed by the framework. If you reorder the tests in the outer script, things break. - Use fixtures. The framework allocates the area for you before each test, calls your init function on it, runs the test, then tears down the area. No dependency between tests. Also, if you have N cases in your test series, you still allocate at most 1 area at any point (as opposed to at most N, like under option #1). > >> +int main(int argc, char **argv) >> +{ >> + TestData data; >> + int ret; >> + >> + g_test_init(&argc, &argv, NULL); >> + >> data.num_cpus = 1; >> >> g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults); >> g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam); >> + add_firmware_test("/i440fx/firmware/bios", request_bios); >> + add_firmware_test("/i440fx/firmware/pflash", request_pflash); >> >> ret = g_test_run(); >> return ret; Thanks Laszlo