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

[Laszlo Ersek <lersek@redhat.com>]

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