Re: [PATCH v2 2/2] char-file: add test for distinct path= and pathin=

[Darren Kenny]

Hi Alex,

For the most part this looks fine, but I wonder if maybe there should be
a couple more assertions to be certain that things are set up correctly
at first, as well as maybe being sure to confirm that things weren't
modified using stat().

See below...

On Thursday, 2020-05-07 at 02:24:42 -04, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov <address@hidden>
> ---
>  tests/test-char.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index 3afc9b1b8d..9b3e1e2a9b 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -1228,6 +1228,88 @@ static void char_file_test_internal(Chardev *ext_chr, 
> const char *filepath)
>      g_free(out);
>  }
>  
> +static int file_can_read(void *opaque)
> +{
> +    return 4096;
> +}
> +
> +static void file_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    int ret;
> +    Chardev *chr = *(Chardev **)opaque;
> +    g_assert_cmpint(size, <=, file_can_read(opaque));
> +
> +    g_assert_cmpint(size, ==, 6);
> +    g_assert(strncmp((const char *)buf, "hello!", 6) == 0);
> +    ret = qemu_chr_write_all(chr, buf, size);
> +    g_assert_cmpint(ret, ==, 6);
> +    quit = true;
> +}
> +
> +static void char_file_separate_input_file(void)
> +{
> +    char *tmp_path = g_dir_make_tmp("qemu-test-char.XXXXXX", NULL);
> +    char *in;
> +    char *out;
> +    QemuOpts *opts;
> +    Chardev *chr;
> +    ChardevFile file = {};
> +    CharBackend be;
> +    ChardevBackend backend = { .type = CHARDEV_BACKEND_KIND_FILE,
> +                               .u.file.data = &file };
> +    char *contents = NULL;
> +    gsize length;
> +    int ret;
> +
> +    in = g_build_filename(tmp_path, "in", NULL);
> +    out = g_build_filename(tmp_path, "out", NULL);
> +
> +    ret = g_file_set_contents(in, "hello!", 6, NULL);

Might be good to confirm that this worked here if we're expecting it to
be correct later.

> +
> +    opts = qemu_opts_create(qemu_find_opts("chardev"), "serial-id",
> +                            1, &error_abort);
> +    qemu_opt_set(opts, "backend", "file", &error_abort);
> +    qemu_opt_set(opts, "pathin", in, &error_abort);
> +    qemu_opt_set(opts, "path", out, &error_abort);
> +
> +    chr = qemu_chr_new_from_opts(opts, NULL, NULL);
> +    qemu_chr_fe_init(&be, chr, &error_abort);
> +
> +    file.has_in = true;
> +    file.in = in;
> +    file.out = out;
> +
> +
> +    qemu_chr_fe_set_handlers(&be, file_can_read,
> +                             file_read,
> +                             NULL, NULL, &chr, NULL, true);
> +
> +    chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
> +                               NULL, &error_abort);

Similarly, maybe confirm here that chr is valid.

> +    main_loop(); /* should call file_read, and copy contents of in to out */
> +
> +    qemu_chr_fe_deinit(&be, true);
> +
> +    /* Check that contents of in were copied to out */
> +    ret = g_file_get_contents(out, &contents, &length, NULL);
> +    g_assert(ret == TRUE);
> +    g_assert_cmpint(length, ==, 6);
> +    g_assert(strncmp(contents, "hello!", 6) == 0);
> +    g_free(contents);
> +
> +    /* Check that in hasn't changed */
> +    ret = g_file_get_contents(in, &contents, &length, NULL);

While this tells us that the content is the same, it doesn't guarantee
that it wasn't modified - it might be worth doing a stat(), or g_stat(),
to ensure that the creation and modifications times are the same?
(Assuming that g_file_set_contents() will do that, or we compare a
before and after struct stat, if not)

I've seen cases in other things where the file was been re-written
exactly the same but only noticed because Vim told me it was externally
modified when I wasn't expecting it have been.

> +    g_assert(ret == TRUE);
> +    g_assert_cmpint(length, ==, 6);
> +    g_assert(strncmp(contents, "hello!", 6) == 0);
> +
> +    g_free(contents);
> +    g_rmdir(tmp_path);
> +    g_free(tmp_path);
> +    g_free(in);
> +    g_free(out);
> +}
> +
>  static void char_file_test(void)
>  {
>      char_file_test_internal(NULL, NULL);
> @@ -1398,6 +1480,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/char/pipe", char_pipe_test);
>  #endif
>      g_test_add_func("/char/file", char_file_test);
> +    g_test_add_func("/char/file/pathin", char_file_separate_input_file);
>  #ifndef _WIN32
>      g_test_add_func("/char/file-fifo", char_file_fifo_test);
>  #endif
> -- 
> 2.26.2

Thanks,

Darren.