From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmPf3-0008Dg-CB for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:07:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VmPex-0002ZD-AR for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:07:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:6895) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmPex-0002Z5-2v for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:07:11 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rATF79XK004867 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 29 Nov 2013 10:07:10 -0500 Message-ID: <5298AD9C.50001@redhat.com> Date: Fri, 29 Nov 2013 16:07:08 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1385662155-15212-1-git-send-email-lersek@redhat.com> <1385662155-15212-4-git-send-email-lersek@redhat.com> <87y547l39a.fsf@blackfin.pond.sub.org> In-Reply-To: <87y547l39a.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/4] i440fx-test: generate temporary firmware blob List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On 11/29/13 14:57, Markus Armbruster wrote: > Laszlo Ersek writes: > >> The blob is 64K in size and contains 0x00..0xFF repeatedly. >> >> The client code added to main() wouldn't make much sense in the long term. >> It helps with debugging and it silences gcc about create_firmware() being >> unused, and we'll replace it in the next patch anyway. >> >> Signed-off-by: Laszlo Ersek >> --- >> tests/i440fx-test.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 62 insertions(+) >> >> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c >> index 3962bca..5506421 100644 >> --- a/tests/i440fx-test.c >> +++ b/tests/i440fx-test.c >> @@ -20,6 +20,11 @@ >> >> #include >> #include >> +#include >> +#include >> +#include >> +#include >> +#include >> >> #define BROKEN 1 >> >> @@ -272,13 +277,70 @@ static void test_i440fx_pam(gconstpointer opaque) >> qtest_end(); >> } >> >> +#define FW_SIZE ((size_t)65536) > > Any particular reason for the cast? Yes. It is a size. It is used in the controlling expression of a for() statement, where the loop variable is a subscript. The variable is size_t too (as it should be). > >> + >> +/* Create a temporary firmware blob, and return its absolute pathname as a >> + * dynamically allocated string. >> + * The file is closed before the function returns; it should be unlinked after >> + * use. >> + * In case of error, NULL is returned. The function prints the error message. >> + */ > > Actually, this creates a blob file. Its temporariness and firmware-ness > are all the caller's business. Rephrase accordingly? I think that would be overkill. The function has a specific use. > > Could this function be generally useful for tests? Not sure. > >> +static char *create_firmware(void) >> +{ >> + int ret, fd; >> + char *pathname; >> + GError *error = NULL; >> + >> + ret = -1; >> + fd = g_file_open_tmp("fw_blob_XXXXXX", &pathname, &error); >> + if (fd == -1) { >> + fprintf(stderr, "unable to create temporary firmware blob: %s\n", >> + error->message); >> + g_error_free(error); >> + } else { >> + if (ftruncate(fd, FW_SIZE) == -1) { >> + fprintf(stderr, "ftruncate(\"%s\", %zu): %s\n", pathname, FW_SIZE, >> + strerror(errno)); > > I wonder whether glib wants us to use g_test_message() here. g_test_message()s are normally supressed, with and without gtester. With gtester, even --verbose doesn't display them (in the default config). "tests/i440fx-test --verbose" displays those messages. This is an error message and I didn't want it to depend on gtester settings or command line arguments. > >> + } else { >> + void *buf; >> + >> + buf = mmap(NULL, FW_SIZE, PROT_WRITE, MAP_SHARED, fd, 0); >> + if (buf == MAP_FAILED) { >> + fprintf(stderr, "mmap(\"%s\", %zu): %s\n", pathname, FW_SIZE, >> + strerror(errno)); >> + } else { >> + size_t i; >> + >> + for (i = 0; i < FW_SIZE; ++i) { >> + ((char unsigned *)buf)[i] = i; > > (unsigned char *), please > > Why not simply unsigned char *buf? Because mmap() returns a (void*), and I need to compare that against MAP_FAILED. The header shall define the symbolic constant MAP_FAILED which shall have type void * and shall be used to indicate a failure from the mmap() function . Your suggestion would include automatic conversion of MAP_FAILED to (char *), which I did not want. > >> + } >> + munmap(buf, FW_SIZE); >> + ret = 0; >> + } >> + } > > Not sure I like this use of mmap(), but it's certainly covered by your > artistic license. Well, thanks for recognizing that. Do you think you could extend your leniency to "char unsigned" as well? My reason for writing these types in this order (char unsigned, long unsigned, long long unsigned) is to follow printf() format specifiers. As in , "%lu", "%llu". (Char types are promoted so no extra width specifiers for them in printf(), but I like to be consistent with myself.) > >> + close(fd); >> + if (ret == -1) { >> + unlink(pathname); >> + g_free(pathname); >> + } >> + } >> + >> + return ret == -1 ? NULL : pathname; >> +} > > Works. Stylistic nitpick: I'd do the error handling differently, > though. I prefer > > if fail > report > bail out > continue normally > > to > > if fail > report > else > continue normally > > because it keeps the normal workings flowing down "straight" rather than > increasingly indented. Normally I'm OK with cascading goto's and/or early returns. I think that the way I did it here matches this situation well. After the g_file_open_tmp() call succeeds, we must close fd in any case (independently of whether as a whole the function succeeds or not). Optionally, we must also unlink the file, in the same logical spot where the close() is. (Because g_file_open() creates three resources at once, a node in the filesystem, a file descriptor in the process, and a dynamically allocated string.) > > static char *create_firmware(void) > { > int fd, i; > char *pathname; > GError *error = NULL; > unsigned char *buf; > > fd = g_file_open_tmp("fw_blob_XXXXXX", &pathname, &error); > g_assert_no_error(error); > > if (ftruncate(fd, FW_SIZE) < 0) { > fprintf(stderr, "ftruncate(\"%s\", %zu): %s\n", pathname, FW_SIZE, > strerror(errno)); > goto fail; > } > > buf = mmap(NULL, FW_SIZE, PROT_WRITE, MAP_SHARED, fd, 0); > if (buf == MAP_FAILED) { > fprintf(stderr, "mmap(\"%s\", %zu): %s\n", pathname, FW_SIZE, > strerror(errno)); > goto fail; > } > > for (i = 0; i < FW_SIZE; ++i) { > buf[i] = i; > } > munmap(buf, FW_SIZE); > > close(fd); > return pathname; > > err: > close(fd); > unlink(pathname); > g_free(pathname); > return NULL; > } Your proposal duplicates the close(fd) call. > >> + >> int main(int argc, char **argv) >> { >> + char *fw_pathname; >> TestData data; >> int ret; >> >> g_test_init(&argc, &argv, NULL); >> >> + fw_pathname = create_firmware(); >> + g_assert(fw_pathname != NULL); >> + unlink(fw_pathname); >> + g_free(fw_pathname); >> + >> data.num_cpus = 1; >> >> g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults); I'm sure you're not deliberately trying to destroy my will to contribute to upstream qemu. Thanks Laszlo