Re: [PATCH v1 4/5] Added tests for close and change of logfile.

[Alex Bennée]

Robert Foley <address@hidden> writes:

> One test ensures that the logfile handle is still valid even if
> the logfile is changed during logging.
> The other test validates that the logfile handle remains valid under
> the logfile lock even if the logfile is closed.
>
> Signed-off-by: Robert Foley <address@hidden>

Reviewed-by: Alex Bennée <address@hidden>

> --
> v1
>     - Changes for first round of code review comments.
>     - Added in use of g_autofree, removed the g_free()s.
>     - Added in use of logfile2 and changed sequence of asserts.
> ---
>  tests/test-logging.c | 80 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>
> diff --git a/tests/test-logging.c b/tests/test-logging.c
> index a12585f70a..1e646f045d 100644
> --- a/tests/test-logging.c
> +++ b/tests/test-logging.c
> @@ -108,6 +108,82 @@ static void test_parse_path(gconstpointer data)
>      error_free_or_abort(&err);
>  }
>
> +static void test_logfile_write(gconstpointer data)
> +{
> +    QemuLogFile *logfile;
> +    QemuLogFile *logfile2;
> +    gchar const *dir = data;
> +    Error *err = NULL;
> +    g_autofree gchar *file_path;
> +    g_autofree gchar *file_path1;
> +    FILE *orig_fd;
> +
> +    /*
> +     * Before starting test, set log flags, to ensure the file gets
> +     * opened below with the call to qemu_set_log_filename().
> +     * In cases where a logging backend other than log is used,
> +     * this is needed.
> +     */
> +    qemu_set_log(CPU_LOG_TB_OUT_ASM);
> +    file_path = g_build_filename(dir, "qemu_test_log_write0.log", NULL);
> +    file_path1 = g_build_filename(dir, "qemu_test_log_write1.log", NULL);
> +
> +    /*
> +     * Test that even if an open file handle is changed,
> +     * our handle remains valid due to RCU.
> +     */
> +    qemu_set_log_filename(file_path, &err);
> +    g_assert(!err);
> +    rcu_read_lock();
> +    logfile = atomic_rcu_read(&qemu_logfile);
> +    orig_fd = logfile->fd;
> +    g_assert(logfile && logfile->fd);
> +    fprintf(logfile->fd, "%s 1st write to file\n", __func__);
> +    fflush(logfile->fd);
> +
> +    /* Change the logfile and ensure that the handle is still valid. */
> +    qemu_set_log_filename(file_path1, &err);
> +    g_assert(!err);
> +    logfile2 = atomic_rcu_read(&qemu_logfile);
> +    g_assert(logfile->fd == orig_fd);
> +    g_assert(logfile2->fd != logfile->fd);
> +    fprintf(logfile->fd, "%s 2nd write to file\n", __func__);
> +    fflush(logfile->fd);
> +    rcu_read_unlock();
> +}
> +
> +static void test_logfile_lock(gconstpointer data)
> +{
> +    FILE *logfile;
> +    gchar const *dir = data;
> +    Error *err = NULL;
> +    g_autofree gchar *file_path;
> +
> +    file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL);
> +
> +    /*
> +     * Test the use of the logfile lock, such
> +     * that even if an open file handle is closed,
> +     * our handle remains valid for use due to RCU.
> +     */
> +    qemu_set_log_filename(file_path, &err);
> +    logfile = qemu_log_lock();
> +    g_assert(logfile);
> +    fprintf(logfile, "%s 1st write to file\n", __func__);
> +    fflush(logfile);
> +
> +    /*
> +     * Initiate a close file and make sure our handle remains
> +     * valid since we still have the logfile lock.
> +     */
> +    qemu_log_close();
> +    fprintf(logfile, "%s 2nd write to file\n", __func__);
> +    fflush(logfile);
> +    qemu_log_unlock(logfile);
> +
> +    g_assert(!err);
> +}
> +
>  /* Remove a directory and all its entries (non-recursive). */
>  static void rmdir_full(gchar const *root)
>  {
> @@ -134,6 +210,10 @@ int main(int argc, char **argv)
>
>      g_test_add_func("/logging/parse_range", test_parse_range);
>      g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path);
> +    g_test_add_data_func("/logging/logfile_write_path",
> +                         tmp_path, test_logfile_write);
> +    g_test_add_data_func("/logging/logfile_lock_path",
> +                         tmp_path, test_logfile_lock);
>
>      rc = g_test_run();


--
Alex Bennée