[Qemu-devel] [PATCH 0/3] char: fix chardev aliases regression

[Qemu-devel] [PATCH 0/3] char: fix chardev aliases regression

[Marc-André Lureau]

Hi,

The patch "char: move CharBackend handling in char-fe unit" broke
chardev aliases. Here is a small series to fix it, and add a simple
unit test to check the alias keep working.

Marc-André Lureau (3):
  char: fix alias devices regression
  chardev: don't use alias names in parse_compat()
  test-char: start a /char/serial test

 chardev/char.c    |  6 ++++--
 tests/test-char.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

-- 
2.13.0.91.g00982b8dd

[Qemu-devel] [PATCH 1/3] char: fix alias devices regression

[Marc-André Lureau]

Fix regression from commit 4d43a603c71, where the serial and parallel
headers got removed from char.c, which broke the alias table.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/chardev/char.c b/chardev/char.c
index 7aa0210765..f38fac5c6b 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -34,6 +34,8 @@
 #include "qemu/help_option.h"
 
 #include "chardev/char-mux.h"
+#include "chardev/char-parallel.h" /* for HAVE_CHARDEV_PARPORT */
+#include "chardev/char-serial.h" /* for HAVE_CHARDEV_SERIAL */
 
 /***********************************************************/
 /* character device */
-- 
2.13.0.91.g00982b8dd

[Qemu-devel] [PATCH 2/3] chardev: don't use alias names in parse_compat()

[Marc-André Lureau]

"parport" is considered "old" since commit 88a946d32d, when "parallel"
was added. Similarly for "tty" in commit d59044ef74d.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index f38fac5c6b..55b671973b 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -452,12 +452,12 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
     }
     if (strstart(filename, "/dev/parport", NULL) ||
         strstart(filename, "/dev/ppi", NULL)) {
-        qemu_opt_set(opts, "backend", "parport", &error_abort);
+        qemu_opt_set(opts, "backend", "parallel", &error_abort);
         qemu_opt_set(opts, "path", filename, &error_abort);
         return opts;
     }
     if (strstart(filename, "/dev/", NULL)) {
-        qemu_opt_set(opts, "backend", "tty", &error_abort);
+        qemu_opt_set(opts, "backend", "serial", &error_abort);
         qemu_opt_set(opts, "path", filename, &error_abort);
         return opts;
     }
-- 
2.13.0.91.g00982b8dd

[Qemu-devel] [PATCH 3/3] test-char: start a /char/serial test

[Marc-André Lureau]

Quite limited test, to check that the chardev can be created with a
path and with the tty alias.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-char.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/test-char.c b/tests/test-char.c
index dfe856cb85..ecc3fec194 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -5,6 +5,7 @@
 #include "qemu/config-file.h"
 #include "qemu/sockets.h"
 #include "chardev/char-fe.h"
+#include "chardev/char-serial.h" /* for HAVE_CHARDEV_SERIAL */
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qom/qom-qobject.h"
@@ -450,6 +451,32 @@ static void char_udp_test(void)
     g_free(tmp);
 }
 
+#ifdef HAVE_CHARDEV_SERIAL
+static void char_serial_test(void)
+{
+    QemuOpts *opts;
+    Chardev *chr;
+
+    opts = qemu_opts_create(qemu_find_opts("chardev"), "serial-id",
+                            1, &error_abort);
+    qemu_opt_set(opts, "backend", "serial", &error_abort);
+    qemu_opt_set(opts, "path", "/dev/null", &error_abort);
+
+    chr = qemu_chr_new_from_opts(opts, NULL);
+    g_assert_nonnull(chr);
+    /* TODO: add more tests with a pty */
+    object_unparent(OBJECT(chr));
+
+    /* test tty alias */
+    qemu_opt_set(opts, "backend", "tty", &error_abort);
+    chr = qemu_chr_new_from_opts(opts, NULL);
+    g_assert_nonnull(chr);
+    object_unparent(OBJECT(chr));
+
+    qemu_opts_del(opts);
+}
+#endif
+
 static void char_file_test(void)
 {
     char *tmp_path = g_dir_make_tmp("qemu-test-char.XXXXXX", NULL);
@@ -597,6 +624,9 @@ int main(int argc, char **argv)
     g_test_add_func("/char/file", char_file_test);
     g_test_add_func("/char/socket", char_socket_test);
     g_test_add_func("/char/udp", char_udp_test);
+#ifdef HAVE_CHARDEV_SERIAL
+    g_test_add_func("/char/serial", char_serial_test);
+#endif
 
     return g_test_run();
 }
-- 
2.13.0.91.g00982b8dd

Re: [Qemu-devel] [PATCH 0/3] char: fix chardev aliases regression

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

On 06/07/2017 01:49 PM, Marc-André Lureau wrote:
> Hi,
> 
> The patch "char: move CharBackend handling in char-fe unit" broke
> chardev aliases. Here is a small series to fix it, and add a simple
> unit test to check the alias keep working.
> 
> Marc-André Lureau (3):
>   char: fix alias devices regression
>   chardev: don't use alias names in parse_compat()
>   test-char: start a /char/serial test
> 
>  chardev/char.c    |  6 ++++--
>  tests/test-char.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] char: fix alias devices regression

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Fix regression from commit 4d43a603c71, where the serial and parallel
> headers got removed from char.c, which broke the alias table.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 7aa0210765..f38fac5c6b 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -34,6 +34,8 @@
>  #include "qemu/help_option.h"
>  
>  #include "chardev/char-mux.h"
> +#include "chardev/char-parallel.h" /* for HAVE_CHARDEV_PARPORT */
> +#include "chardev/char-serial.h" /* for HAVE_CHARDEV_SERIAL */
>  
>  /***********************************************************/
>  /* character device */

Two drive-by observations:

* Putting HAVE_FOOs in random headers, then testing them with #ifdef is
  asking for trouble.  Anything you test with #ifdef should be there
  after #include "qemu/osdep.h" at the latest, or be defined in the same
  .c.

* Such comments after #include rot quickly.  Strong dislike.

Doesn't mean this isn't an acceptable minimally invasive regression fix.

Re: [Qemu-devel] [PATCH 1/3] char: fix alias devices regression

[Marc-André Lureau]

Hi

----- Original Message -----
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > Fix regression from commit 4d43a603c71, where the serial and parallel
> > headers got removed from char.c, which broke the alias table.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  chardev/char.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/chardev/char.c b/chardev/char.c
> > index 7aa0210765..f38fac5c6b 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -34,6 +34,8 @@
> >  #include "qemu/help_option.h"
> >  
> >  #include "chardev/char-mux.h"
> > +#include "chardev/char-parallel.h" /* for HAVE_CHARDEV_PARPORT */
> > +#include "chardev/char-serial.h" /* for HAVE_CHARDEV_SERIAL */
> >  
> >  /***********************************************************/
> >  /* character device */
> 
> Two drive-by observations:
> 
> * Putting HAVE_FOOs in random headers, then testing them with #ifdef is
>   asking for trouble.  Anything you test with #ifdef should be there
>   after #include "qemu/osdep.h" at the latest, or be defined in the same
>   .c.
> 

I agree, is it fine to move those HAVE_FOO there?

> * Such comments after #include rot quickly.  Strong dislike.

Well, if that comment would have been there, there would have been less chance to remove the header by mistake, even if it rotted.

Re: [Qemu-devel] [PATCH 1/3] char: fix alias devices regression

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> ----- Original Message -----
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>> 
>> > Fix regression from commit 4d43a603c71, where the serial and parallel
>> > headers got removed from char.c, which broke the alias table.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  chardev/char.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/chardev/char.c b/chardev/char.c
>> > index 7aa0210765..f38fac5c6b 100644
>> > --- a/chardev/char.c
>> > +++ b/chardev/char.c
>> > @@ -34,6 +34,8 @@
>> >  #include "qemu/help_option.h"
>> >  
>> >  #include "chardev/char-mux.h"
>> > +#include "chardev/char-parallel.h" /* for HAVE_CHARDEV_PARPORT */
>> > +#include "chardev/char-serial.h" /* for HAVE_CHARDEV_SERIAL */
>> >  
>> >  /***********************************************************/
>> >  /* character device */
>> 
>> Two drive-by observations:
>> 
>> * Putting HAVE_FOOs in random headers, then testing them with #ifdef is
>>   asking for trouble.  Anything you test with #ifdef should be there
>>   after #include "qemu/osdep.h" at the latest, or be defined in the same
>>   .c.
>> 
>
> I agree, is it fine to move those HAVE_FOO there?

Let's have a look at the definitions.

    #if defined(__linux__) || defined(__FreeBSD__) ||               \
        defined(__FreeBSD_kernel__) || defined(__DragonFly__)
    #define HAVE_CHARDEV_PARPORT 1
    #endif

Used in char-parallel.c and char.c.  If it was used in just one of them,
I'd ask you to move it there.

    #ifdef _WIN32
    #define HAVE_CHARDEV_SERIAL 1
    #elif defined(__linux__) || defined(__sun__) || defined(__FreeBSD__)    \
        || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \
        || defined(__GLIBC__)
    #define HAVE_CHARDEV_SERIAL 1
    #endif

Used in char-serial.c and char.c.  Same story.

osdep.h seems like fair game to me.  Precedence: QEMU_VMALLOC_ALIGN.

>> * Such comments after #include rot quickly.  Strong dislike.
>
> Well, if that comment would have been there, there would have been less chance to remove the header by mistake, even if it rotted.

Point taken.  So I dislike a quick-rotting work-around for a bad idea ;)