Re: [Qemu-devel] [PATCH v2 3/4] i440fx-test: generate temporary firmware blob

[Markus Armbruster]

Laszlo Ersek <address@hidden> 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 <address@hidden>
> ---
>  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);