[PATCH v3 0/2] Add pathin option to -chardev file

[PATCH v3 0/2] Add pathin option to -chardev file

[Alexander Bulekov]

This adds a pathin= option to -chardev file, which allows specifying
distinct input and output paths for the chardev. This functionaliy was
already available through QMP.

v3:
  * char-test: add a stat()-based check to ensure that the pathin=
    file is not modified during the execution of the test.


Alexander Bulekov (2):
  chardev: enable distinct input for -chardev file
  char-file: add test for distinct path= and pathin=

 chardev/char-file.c |  5 +++
 chardev/char.c      |  3 ++
 qemu-options.hx     |  7 +++-
 tests/test-char.c   | 96 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 109 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH v3 1/2] chardev: enable distinct input for -chardev file

[Alexander Bulekov]

char-file already supports distinct paths for input/output but it was
only possible to specify a distinct input through QMP. With this change,
we can also specify a distinct input with the -chardev file argument:
    qemu -chardev file,id=char1,path=/out/file,pathin=/in/file

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
---
 chardev/char-file.c | 5 +++++
 chardev/char.c      | 3 +++
 qemu-options.hx     | 7 +++++--
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/chardev/char-file.c b/chardev/char-file.c
index 2fd80707e5..031f2aa7d7 100644
--- a/chardev/char-file.c
+++ b/chardev/char-file.c
@@ -100,6 +100,7 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend,
                                     Error **errp)
 {
     const char *path = qemu_opt_get(opts, "path");
+    const char *pathin = qemu_opt_get(opts, "pathin");
     ChardevFile *file;
 
     backend->type = CHARDEV_BACKEND_KIND_FILE;
@@ -110,6 +111,10 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend,
     file = backend->u.file.data = g_new0(ChardevFile, 1);
     qemu_chr_parse_common(opts, qapi_ChardevFile_base(file));
     file->out = g_strdup(path);
+    if (pathin) {
+        file->has_in = true;
+        file->in = g_strdup(pathin);
+    }
 
     file->has_append = true;
     file->append = qemu_opt_get_bool(opts, "append", false);
diff --git a/chardev/char.c b/chardev/char.c
index e77564060d..97e03a8e48 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -849,6 +849,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "path",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "pathin",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "host",
             .type = QEMU_OPT_STRING,
diff --git a/qemu-options.hx b/qemu-options.hx
index 292d4e7c0c..488961099b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2938,7 +2938,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
     "-chardev vc,id=id[[,width=width][,height=height]][[,cols=cols][,rows=rows]]\n"
     "         [,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
     "-chardev ringbuf,id=id[,size=size][,logfile=PATH][,logappend=on|off]\n"
-    "-chardev file,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
+    "-chardev file,id=id,path=path[,pathin=PATH][,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
     "-chardev pipe,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
 #ifdef _WIN32
     "-chardev console,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
@@ -3137,13 +3137,16 @@ The available backends are:
     Create a ring buffer with fixed size ``size``. size must be a power
     of two and defaults to ``64K``.
 
-``-chardev file,id=id,path=path``
+``-chardev file,id=id,path=path[,pathin=pathin]``
     Log all traffic received from the guest to a file.
 
     ``path`` specifies the path of the file to be opened. This file will
     be created if it does not already exist, and overwritten if it does.
     ``path`` is required.
 
+    ``pathin`` specifies a separate file as the input to the chardev. If
+    ``pathin`` is omitted, ``path`` is used for both input and output
+
 ``-chardev pipe,id=id,path=path``
     Create a two-way connection to the guest. The behaviour differs
     slightly between Windows hosts and other hosts:
-- 
2.26.2

Re: [PATCH v3 1/2] chardev: enable distinct input for -chardev file

[Markus Armbruster]

Alexander Bulekov <alxndr@bu.edu> writes:

> char-file already supports distinct paths for input/output but it was
> only possible to specify a distinct input through QMP. With this change,
> we can also specify a distinct input with the -chardev file argument:
>     qemu -chardev file,id=char1,path=/out/file,pathin=/in/file
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
>  chardev/char-file.c | 5 +++++
>  chardev/char.c      | 3 +++
>  qemu-options.hx     | 7 +++++--
>  3 files changed, 13 insertions(+), 2 deletions(-)
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 292d4e7c0c..488961099b 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2938,7 +2938,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
>      "-chardev vc,id=id[[,width=width][,height=height]][[,cols=cols][,rows=rows]]\n"
>      "         [,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>      "-chardev ringbuf,id=id[,size=size][,logfile=PATH][,logappend=on|off]\n"
> -    "-chardev file,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> +    "-chardev file,id=id,path=path[,pathin=PATH][,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>      "-chardev pipe,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>  #ifdef _WIN32
>      "-chardev console,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> @@ -3137,13 +3137,16 @@ The available backends are:
>      Create a ring buffer with fixed size ``size``. size must be a power
>      of two and defaults to ``64K``.
>  
> -``-chardev file,id=id,path=path``
> +``-chardev file,id=id,path=path[,pathin=pathin]``
>      Log all traffic received from the guest to a file.
>  
>      ``path`` specifies the path of the file to be opened. This file will
>      be created if it does not already exist, and overwritten if it does.
>      ``path`` is required.
>  
> +    ``pathin`` specifies a separate file as the input to the chardev. If
> +    ``pathin`` is omitted, ``path`` is used for both input and output
> +
>  ``-chardev pipe,id=id,path=path``
>      Create a two-way connection to the guest. The behaviour differs
>      slightly between Windows hosts and other hosts:

"pathin" is ugly.  What about "path-input"?

Re: [PATCH v3 1/2] chardev: enable distinct input for -chardev file

[Alexander Bulekov]

On 200515 0526, Markus Armbruster wrote:
> Alexander Bulekov <alxndr@bu.edu> writes:
> 
> >  ``-chardev pipe,id=id,path=path``
> >      Create a two-way connection to the guest. The behaviour differs
> >      slightly between Windows hosts and other hosts:
> 
> "pathin" is ugly.  What about "path-input"?

Sounds better - I'll send another version out.
-Alex

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

[Alexander Bulekov]

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/test-char.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/tests/test-char.c b/tests/test-char.c
index 3afc9b1b8d..6c66fae86a 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -1228,6 +1228,101 @@ 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, (const uint8_t *)"world!", 6);
+    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;
+    time_t in_mtime;
+    GStatBuf file_stat;
+
+    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);
+    g_assert(ret == TRUE);
+    g_stat(in, &file_stat);
+    in_mtime = file_stat.st_mtime;
+    /*
+     * Sleep to ensure that if the following actions modify the file, the mtime
+     * will be different
+     */
+    sleep(1);
+    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);
+    g_assert_nonnull(chr);
+
+    main_loop(); /* should call file_read, and copy contents of in to out */
+
+    qemu_chr_fe_deinit(&be, true);
+
+    /* Check that out was written to */
+    ret = g_file_get_contents(out, &contents, &length, NULL);
+    g_assert(ret == TRUE);
+    g_assert_cmpint(length, ==, 6);
+    g_assert(strncmp(contents, "world!", 6) == 0);
+    g_free(contents);
+
+    /* Check that in hasn't been modified */
+    ret = g_file_get_contents(in, &contents, &length, NULL);
+    g_assert(ret == TRUE);
+    g_assert_cmpint(length, ==, 6);
+    g_assert(strncmp(contents, "hello!", 6) == 0);
+    g_stat(in, &file_stat);
+    g_assert(file_stat.st_mtime == in_mtime);
+
+    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 +1493,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

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

[Darren Kenny]

Hi Alex,

On Monday, 2020-05-11 at 23:47:50 -04, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Thanks for making those changes.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

> ---
>  tests/test-char.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index 3afc9b1b8d..6c66fae86a 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -1228,6 +1228,101 @@ 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, (const uint8_t *)"world!", 6);
> +    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;
> +    time_t in_mtime;
> +    GStatBuf file_stat;
> +
> +    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);
> +    g_assert(ret == TRUE);
> +    g_stat(in, &file_stat);
> +    in_mtime = file_stat.st_mtime;
> +    /*
> +     * Sleep to ensure that if the following actions modify the file, the mtime
> +     * will be different
> +     */
> +    sleep(1);
> +    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);
> +    g_assert_nonnull(chr);
> +
> +    main_loop(); /* should call file_read, and copy contents of in to out */
> +
> +    qemu_chr_fe_deinit(&be, true);
> +
> +    /* Check that out was written to */
> +    ret = g_file_get_contents(out, &contents, &length, NULL);
> +    g_assert(ret == TRUE);
> +    g_assert_cmpint(length, ==, 6);
> +    g_assert(strncmp(contents, "world!", 6) == 0);
> +    g_free(contents);
> +
> +    /* Check that in hasn't been modified */
> +    ret = g_file_get_contents(in, &contents, &length, NULL);
> +    g_assert(ret == TRUE);
> +    g_assert_cmpint(length, ==, 6);
> +    g_assert(strncmp(contents, "hello!", 6) == 0);
> +    g_stat(in, &file_stat);
> +    g_assert(file_stat.st_mtime == in_mtime);
> +
> +    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 +1493,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