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

[PATCH v2 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.

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   | 83 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH v2 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>
---
 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 v2 1/2] chardev: enable distinct input for -chardev file

[Darren Kenny]

On Thursday, 2020-05-07 at 02:24:41 -04, Alexander Bulekov wrote:
> 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

[PATCH v2 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 | 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);
+
+    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);
+    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);
+    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

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 <alxndr@bu.edu>
> ---
>  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.