Laszlo Ersek <ler...@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 <ler...@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);
