Dear Peter,
Peter Maydell <peter.mayd...@linaro.org> writes:
> On 15 July 2016 at 17:24, Sascha Silbe <si...@linux.vnet.ibm.com> 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 <si...@linux.vnet.ibm.com>
>
> 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
