| Message ID | 1385662155-15212-4-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com> > --- > 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 <glib.h> > #include <string.h> > +#include <stdio.h> > +#include <unistd.h> > +#include <errno.h> > +#include <sys/mman.h> > +#include <stdlib.h> > > #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? > + > +/* 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? Could this function be generally useful for tests? > +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. > + } 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? > + } > + munmap(buf, FW_SIZE); > + ret = 0; > + } > + } Not sure I like this use of mmap(), but it's certainly covered by your artistic license. > + 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. 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; } > + > 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);
On 11/29/13 14:57, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com> >> --- >> 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 <glib.h> >> #include <string.h> >> +#include <stdio.h> >> +#include <unistd.h> >> +#include <errno.h> >> +#include <sys/mman.h> >> +#include <stdlib.h> >> >> #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 <sys/mman.h> 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
Il 29/11/2013 16:07, Laszlo Ersek ha scritto: > 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.) I agree that the way you wrote it makes sense if you do not use goto. Paolo
Laszlo Ersek <lersek@redhat.com> writes: > On 11/29/13 14:57, Markus Armbruster wrote: >> Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com> >>> --- >>> 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 <glib.h> >>> #include <string.h> >>> +#include <stdio.h> >>> +#include <unistd.h> >>> +#include <errno.h> >>> +#include <sys/mman.h> >>> +#include <stdlib.h> >>> >>> #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). I disagree, but it's your code. >>> + >>> +/* 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. Makes sense. Does glib provide means to report test errors other than g_assert() & friends? I'm asking because this would be the first use of stderr in tests/*-test.c. Losely related: should we embrace the fact that this is a test function, whose failure is always ultimately fatal, and terminate the test right there? >> >>> + } 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 <sys/mman.h> 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. This isn't C++. Generic pointers are a feature of C. I don't think littering the code with casts to avoid them is a good idea, because 1. casts defeat the type checker, and 2. brevity is a virtue. >>> + } >>> + 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.) I prefer to be consistent with the rest of the code $ git-grep 'unsigned char'|wc -l 553 $ git-grep 'char unsigned'|wc -l 8 even when I find it rather disagreable, like CamelCasedTypeDefs. Besides, C uses words borrowed from English, so sticking to English grammar (unsigned character) rather than French grammar (charactère non-signée) makes some sense. My brain parses the familiar "unsigned char" more quickly than the unusual "char unsigned". >>> + 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. Yes, but there's one less variable to keep in short-term memory (ret), and less nesting. Overall, I find it easier to follow. Anyway, your choice. >>> + >>> 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. Nope, and I'm sure you're not deliberately trying to destroy my will to review your upstream QEMU patches. I just re-read my review, and I still believe it was obvious enough that most of my comments where of the type "have you thought of this, perhaps you like it better (I do)". I'm sorry this brushed you the wrong way. Such comments will happen again, I'm afraid. If I knew how to make them more obvious without adding verbiage, I'd try.
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 <glib.h> #include <string.h> +#include <stdio.h> +#include <unistd.h> +#include <errno.h> +#include <sys/mman.h> +#include <stdlib.h> #define BROKEN 1 @@ -272,13 +277,70 @@ static void test_i440fx_pam(gconstpointer opaque) qtest_end(); } +#define FW_SIZE ((size_t)65536) + +/* 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. + */ +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)); + } 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; + } + munmap(buf, FW_SIZE); + ret = 0; + } + } + close(fd); + if (ret == -1) { + unlink(pathname); + g_free(pathname); + } + } + + return ret == -1 ? NULL : pathname; +} + 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);
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 <lersek@redhat.com> --- tests/i440fx-test.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)