Re: [Qemu-devel] [PATCH] test-logging: don't hard-code paths in /tmp

[Sascha Silbe]

Dear Peter,

Peter Maydell <address@hidden> writes:

> On 15 July 2016 at 17:24, Sascha Silbe <address@hidden> wrote:
[...]
>> Instead of hard-coding the paths, create a temporary directory using
>> g_dir_make_tmp() and clean it up afterwards.
>>
>> Fixes: f6880b7f ("qemu-log: support simple pid substitution for logs")
>> Signed-off-by: Sascha Silbe <address@hidden>
>
> Thanks for this patch -- I just noticed that the test was leaving
> temporary files not cleaned up, which brought me to this patch
> by searching the mail archives...

I have totally forgotten about it. Would probably have remembered the
next time "make check" failed on a shared machine. ;)


[tests/test-logging.c:test_parse_path()]
>> +    gchar *plain_path = g_build_filename(tmp_path, "qemu.log", NULL);
>> +    gchar *pid_infix_path = g_build_filename(tmp_path, "qemu-%d.log", NULL);
>> +    gchar *pid_suffix_path = g_build_filename(tmp_path, "qemu.log.%d", 
>> NULL);
>> +    gchar *double_pid_path = g_build_filename(tmp_path, "qemu-%d%d.log", 
>> NULL);
[...]
> This could usefully be refactored to have a helper function that does
> the g_build_filename/qemu_set_log_filename/g_free.

I wasn't sure about it but it actually ended up being a bit nicer than
what I had before.


>> +static void rmtree(gchar const *root)
[...]
> I don't really like spawning rm here for this. We know we
> don't have any subdirectories here so we can just
>    gdir = g_dir_open(root, 0, NULL);
>    for (;;) {
>        const char *f = g_dir_read_name(gdir);
>        if (!f) {
>            break;
>        }
>        g_remove(f);
>    }
>    g_dir_close(gdir);
>    g_rmdir(root);
>
> (add error handling to taste).

I don't really like the rm spawning solution either. But the above plus
error handling was a bit much for a single test for my taste.

Is there some place I could stick something like the above so it could
at least be reused by other tests in the future? (Even though I very
much hate the idea of implementing yet another rmtree()).


[...]
>>  int main(int argc, char **argv)
>>  {
>> +    gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
>
> Unfortunately g_dir_make_tmp() only came in in glib 2.30, and we have
> to be able to build with glib 2.22.

Bummer. The old interfaces were really awkward. Is there somewhere I
could put a compatibility wrapper, implementing g_dir_make_tmp() using
the old interfaces?


Thanks for the review!

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641