[Qemu-devel] [PATCH v2 00/21] qemu-char: refactoring of chardev creation

[Qemu-devel] [PATCH v2 00/21] qemu-char: refactoring of chardev creation

[Paolo Bonzini]

This series rewrites chardev creation to use a new ->create
member of the CharDriver struct, and to always signal errors
via Error*.

The advantage is that backend-specific creation functions need
not be exported anymore for qemu-char.c's usage, and hence do not
need stubs anymore.

Paolo Bonzini (21):
  qemu-char: cleanup qmp_chardev_add
  qemu-char: cleanup HAVE_CHARDEV_*
  qemu-char: add create to register_char_driver
  qemu-char: convert file backend to data-driven creation
  qemu-char: convert serial backend to data-driven creation
  qemu-char: convert parallel backend to data-driven creation
  qemu-char: convert pipe backend to data-driven creation
  qemu-char: convert socket backend to data-driven creation
  qemu-char: convert UDP backend to data-driven creation
  qemu-char: convert pty backend to data-driven creation
  qemu-char: convert null backend to data-driven creation
  qemu-char: convert mux backend to data-driven creation
  qemu-char: convert msmouse backend to data-driven creation
  qemu-char: convert braille backend to data-driven creation
  qemu-char: convert testdev backend to data-driven creation
  qemu-char: convert stdio backend to data-driven creation
  qemu-char: convert console backend to data-driven creation
  qemu-char: convert spice backend to data-driven creation
  qemu-char: convert vc backend to data-driven creation
  qemu-char: convert ringbuf backend to data-driven creation
  qemu-char: cleanup after completed conversion to cd->create

 backends/baum.c             |  17 +-
 backends/msmouse.c          |   8 +-
 backends/testdev.c          |   8 +-
 include/sysemu/char.h       |  18 +-
 include/ui/qemu-spice.h     |   2 -
 qemu-char.c                 | 392 ++++++++++++++++++++++++--------------------
 spice-qemu-char.c           |  21 ++-
 stubs/Makefile.objs         |   5 -
 stubs/chr-baum-init.c       |   7 -
 stubs/chr-msmouse.c         |   7 -
 stubs/chr-testdev.c         |   7 -
 stubs/qemu-chr-open-spice.c |  14 --
 stubs/vc-init.c             |   7 -
 ui/console.c                |  11 +-
 ui/gtk.c                    |   2 +-
 15 files changed, 260 insertions(+), 266 deletions(-)
 delete mode 100644 stubs/chr-baum-init.c
 delete mode 100644 stubs/chr-msmouse.c
 delete mode 100644 stubs/chr-testdev.c
 delete mode 100644 stubs/qemu-chr-open-spice.c
 delete mode 100644 stubs/vc-init.c

-- 
2.5.0

[Qemu-devel] [PATCH v2 06/21] qemu-char: convert parallel backend to data-driven creation

[Paolo Bonzini]

Conversion to Error * brings better error messages; before:

    qemu-system-x86_64: -chardev id=serial,backend=parallel,path=vl.c: Failed to create chardev

After:

    qemu-system-x86_64: -chardev id=serial,backend=parallel,path=vl.c: not a parallel port: Inappropriate ioctl for device

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 8567580..ff7722f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1753,12 +1753,13 @@ static void pp_close(CharDriverState *chr)
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-static CharDriverState *qemu_chr_open_pp_fd(int fd)
+static CharDriverState *qemu_chr_open_pp_fd(int fd, Error **errp)
 {
     CharDriverState *chr;
     ParallelCharDriver *drv;
 
     if (ioctl(fd, PPCLAIM) < 0) {
+        error_setg_errno(errp, errno, "not a parallel port");
         close(fd);
         return NULL;
     }
@@ -1818,7 +1819,7 @@ static int pp_ioctl(CharDriverState *chr, int cmd, void *arg)
     return 0;
 }
 
-static CharDriverState *qemu_chr_open_pp_fd(int fd)
+static CharDriverState *qemu_chr_open_pp_fd(int fd, Error **errp)
 {
     CharDriverState *chr;
 
@@ -3481,6 +3482,7 @@ static void qemu_chr_parse_serial(QemuOpts *opts, ChardevBackend *backend,
 }
 #endif
 
+#ifdef HAVE_CHARDEV_PARPORT
 static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
                                     Error **errp)
 {
@@ -3493,6 +3495,7 @@ static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
     backend->parallel = g_new0(ChardevHostdev, 1);
     backend->parallel->device = g_strdup(device);
 }
+#endif
 
 static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
                                 Error **errp)
@@ -4044,7 +4047,9 @@ static CharDriverState *qmp_chardev_open_serial(const char *id,
     return qemu_chr_open_win_path(serial->device, errp);
 }
 
-static CharDriverState *qmp_chardev_open_parallel(ChardevHostdev *parallel,
+static CharDriverState *qmp_chardev_open_parallel(const char *id,
+                                                  ChardevBackend *backend,
+                                                  ChardevReturn *ret,
                                                   Error **errp)
 {
     error_setg(errp, "character device backend type 'parallel' not supported");
@@ -4110,16 +4115,19 @@ static CharDriverState *qmp_chardev_open_serial(const char *id,
 #endif
 
 #ifdef HAVE_CHARDEV_PARPORT
-static CharDriverState *qmp_chardev_open_parallel(ChardevHostdev *parallel,
+static CharDriverState *qmp_chardev_open_parallel(const char *id,
+                                                  ChardevBackend *backend,
+                                                  ChardevReturn *ret,
                                                   Error **errp)
 {
+    ChardevHostdev *parallel = backend->parallel;
     int fd;
 
     fd = qmp_chardev_open_file_source(parallel->device, O_RDWR, errp);
     if (fd < 0) {
         return NULL;
     }
-    return qemu_chr_open_pp_fd(fd);
+    return qemu_chr_open_pp_fd(fd, errp);
 }
 #endif
 
@@ -4265,11 +4273,9 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         case CHARDEV_BACKEND_KIND_SERIAL:
             abort();
             break;
-#ifdef HAVE_CHARDEV_PARPORT
         case CHARDEV_BACKEND_KIND_PARALLEL:
-            chr = qmp_chardev_open_parallel(backend->parallel, &local_err);
+            abort();
             break;
-#endif
         case CHARDEV_BACKEND_KIND_PIPE:
             chr = qemu_chr_open_pipe(backend->pipe);
             break;
@@ -4405,10 +4411,12 @@ static void register_types(void)
     register_char_driver("tty", CHARDEV_BACKEND_KIND_SERIAL,
                          qemu_chr_parse_serial, qmp_chardev_open_serial);
 #endif
+#ifdef HAVE_CHARDEV_PARPORT
     register_char_driver("parallel", CHARDEV_BACKEND_KIND_PARALLEL,
-                         qemu_chr_parse_parallel, NULL);
+                         qemu_chr_parse_parallel, qmp_chardev_open_parallel);
     register_char_driver("parport", CHARDEV_BACKEND_KIND_PARALLEL,
-                         qemu_chr_parse_parallel, NULL);
+                         qemu_chr_parse_parallel, qmp_chardev_open_parallel);
+#endif
     register_char_driver("pty", CHARDEV_BACKEND_KIND_PTY, NULL,
                          NULL);
     register_char_driver("console", CHARDEV_BACKEND_KIND_CONSOLE, NULL,
-- 
2.5.0

[Qemu-devel] [PATCH v2 07/21] qemu-char: convert pipe backend to data-driven creation

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index ff7722f..47db98f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1078,18 +1078,17 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
     return chr;
 }
 
-static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
+static CharDriverState *qemu_chr_open_pipe(const char *id,
+                                           ChardevBackend *backend,
+                                           ChardevReturn *ret,
+                                           Error **errp)
 {
+    ChardevHostdev *opts = backend->pipe;
     int fd_in, fd_out;
     char filename_in[CHR_MAX_FILENAME_SIZE];
     char filename_out[CHR_MAX_FILENAME_SIZE];
     const char *filename = opts->device;
 
-    if (filename == NULL) {
-        fprintf(stderr, "chardev: pipe: no filename given\n");
-        return NULL;
-    }
-
     snprintf(filename_in, CHR_MAX_FILENAME_SIZE, "%s.in", filename);
     snprintf(filename_out, CHR_MAX_FILENAME_SIZE, "%s.out", filename);
     TFR(fd_in = qemu_open(filename_in, O_RDWR | O_BINARY));
@@ -1101,6 +1100,7 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
 	    close(fd_out);
         TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
         if (fd_in < 0) {
+            error_setg_file_open(errp, errno, filename);
             return NULL;
         }
     }
@@ -2084,7 +2084,8 @@ static int win_chr_pipe_poll(void *opaque)
     return 0;
 }
 
-static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
+static int win_chr_pipe_init(CharDriverState *chr, const char *filename,
+                             Error **errp)
 {
     WinCharState *s = chr->opaque;
     OVERLAPPED ov;
@@ -2096,12 +2097,12 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
 
     s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hsend) {
-        fprintf(stderr, "Failed CreateEvent\n");
+        error_setg(errp, "Failed CreateEvent");
         goto fail;
     }
     s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hrecv) {
-        fprintf(stderr, "Failed CreateEvent\n");
+        error_setg(errp, "Failed CreateEvent");
         goto fail;
     }
 
@@ -2111,7 +2112,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
                               PIPE_WAIT,
                               MAXCONNECT, NSENDBUF, NRECVBUF, NTIMEOUT, NULL);
     if (s->hcom == INVALID_HANDLE_VALUE) {
-        fprintf(stderr, "Failed CreateNamedPipe (%lu)\n", GetLastError());
+        error_setg(errp, "Failed CreateNamedPipe (%lu)", GetLastError());
         s->hcom = NULL;
         goto fail;
     }
@@ -2120,13 +2121,13 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
     ov.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
     ret = ConnectNamedPipe(s->hcom, &ov);
     if (ret) {
-        fprintf(stderr, "Failed ConnectNamedPipe\n");
+        error_setg(errp, "Failed ConnectNamedPipe");
         goto fail;
     }
 
     ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
     if (!ret) {
-        fprintf(stderr, "Failed GetOverlappedResult\n");
+        error_setg(errp, "Failed GetOverlappedResult");
         if (ov.hEvent) {
             CloseHandle(ov.hEvent);
             ov.hEvent = NULL;
@@ -2147,8 +2148,12 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
 }
 
 
-static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
+static CharDriverState *qemu_chr_open_pipe(const char *id,
+                                           ChardevBackend *backend,
+                                           ChardevReturn *ret,
+                                           Error **errp)
 {
+    ChardevHostdev *opts = backend->pipe;
     const char *filename = opts->device;
     CharDriverState *chr;
     WinCharState *s;
@@ -2159,7 +2164,7 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
     chr->chr_write = win_chr_write;
     chr->chr_close = win_chr_close;
 
-    if (win_chr_pipe_init(chr, filename) < 0) {
+    if (win_chr_pipe_init(chr, filename, errp) < 0) {
         g_free(s);
         g_free(chr);
         return NULL;
@@ -4277,7 +4282,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
             abort();
             break;
         case CHARDEV_BACKEND_KIND_PIPE:
-            chr = qemu_chr_open_pipe(backend->pipe);
+            abort();
             break;
         case CHARDEV_BACKEND_KIND_SOCKET:
             chr = qmp_chardev_open_socket(backend->socket, &local_err);
@@ -4422,7 +4427,7 @@ static void register_types(void)
     register_char_driver("console", CHARDEV_BACKEND_KIND_CONSOLE, NULL,
                          NULL);
     register_char_driver("pipe", CHARDEV_BACKEND_KIND_PIPE,
-                         qemu_chr_parse_pipe, NULL);
+                         qemu_chr_parse_pipe, qemu_chr_open_pipe);
     register_char_driver("mux", CHARDEV_BACKEND_KIND_MUX, qemu_chr_parse_mux,
                          NULL);
     /* Bug-compatibility: */
-- 
2.5.0

[Qemu-devel] [PATCH v2 14/21] qemu-char: convert braille backend to data-driven creation

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 backends/baum.c       | 16 +++++++++++-----
 include/sysemu/char.h |  3 ---
 qemu-char.c           |  4 +---
 stubs/Makefile.objs   |  1 -
 stubs/chr-baum-init.c |  7 -------
 5 files changed, 12 insertions(+), 19 deletions(-)
 delete mode 100644 stubs/chr-baum-init.c

diff --git a/backends/baum.c b/backends/baum.c
index e86a019..723c658 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -561,7 +561,10 @@ static void baum_close(struct CharDriverState *chr)
     g_free(baum);
 }
 
-CharDriverState *chr_baum_init(void)
+static CharDriverState *chr_baum_init(const char *id,
+                                      ChardevBackend *backend,
+                                      ChardevReturn *ret,
+                                      Error **errp)
 {
     BaumDriverState *baum;
     CharDriverState *chr;
@@ -586,14 +589,16 @@ CharDriverState *chr_baum_init(void)
 
     baum->brlapi_fd = brlapi__openConnection(handle, NULL, NULL);
     if (baum->brlapi_fd == -1) {
-        brlapi_perror("baum_init: brlapi_openConnection");
+        error_setg(errp, "brlapi__openConnection: %s",
+                   brlapi_strerror(brlapi_error_location()));
         goto fail_handle;
     }
 
     baum->cellCount_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, baum_cellCount_timer_cb, baum);
 
     if (brlapi__getDisplaySize(handle, &baum->x, &baum->y) == -1) {
-        brlapi_perror("baum_init: brlapi_getDisplaySize");
+        error_setg(errp, "brlapi__getDisplaySize: %s",
+                   brlapi_strerror(brlapi_error_location()));
         goto fail;
     }
 
@@ -609,7 +614,8 @@ CharDriverState *chr_baum_init(void)
         tty = BRLAPI_TTY_DEFAULT;
 
     if (brlapi__enterTtyMode(handle, tty, NULL) == -1) {
-        brlapi_perror("baum_init: brlapi_enterTtyMode");
+        error_setg(errp, "brlapi__enterTtyMode: %s",
+                   brlapi_strerror(brlapi_error_location()));
         goto fail;
     }
 
@@ -630,7 +636,7 @@ fail_handle:
 static void register_types(void)
 {
     register_char_driver("braille", CHARDEV_BACKEND_KIND_BRAILLE, NULL,
-                         NULL);
+                         chr_baum_init);
 }
 
 type_init(register_types);
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 2fe8275..77415ec 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -359,9 +359,6 @@ CharDriverState *qemu_char_get_next_serial(void);
 /* testdev.c */
 CharDriverState *chr_testdev_init(void);
 
-/* baum.c */
-CharDriverState *chr_baum_init(void);
-
 /* console.c */
 typedef CharDriverState *(VcHandler)(ChardevVC *vc);
 
diff --git a/qemu-char.c b/qemu-char.c
index 21a50a5..06163bd 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4323,11 +4323,9 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         case CHARDEV_BACKEND_KIND_MSMOUSE:
             abort();
             break;
-#ifdef CONFIG_BRLAPI
         case CHARDEV_BACKEND_KIND_BRAILLE:
-            chr = chr_baum_init();
+            abort();
             break;
-#endif
         case CHARDEV_BACKEND_KIND_TESTDEV:
             chr = chr_testdev_init();
             break;
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 63988ca..8cfa5a2 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,6 +1,5 @@
 stub-obj-y += arch-query-cpu-def.o
 stub-obj-y += bdrv-commit-all.o
-stub-obj-y += chr-baum-init.o
 stub-obj-y += chr-testdev.o
 stub-obj-y += clock-warp.o
 stub-obj-y += cpu-get-clock.o
diff --git a/stubs/chr-baum-init.c b/stubs/chr-baum-init.c
deleted file mode 100644
index f5cc6ce..0000000
--- a/stubs/chr-baum-init.c
+++ /dev/null
@@ -1,7 +0,0 @@
-#include "qemu-common.h"
-#include "sysemu/char.h"
-
-CharDriverState *chr_baum_init(void)
-{
-    return NULL;
-}
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 00/21] qemu-char: refactoring of chardev creation

[Eric Blake]

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

On 10/14/2015 09:54 AM, Paolo Bonzini wrote:
> This series rewrites chardev creation to use a new ->create
> member of the CharDriver struct, and to always signal errors
> via Error*.
> 
> The advantage is that backend-specific creation functions need
> not be exported anymore for qemu-char.c's usage, and hence do not
> need stubs anymore.

Hmm - I just noticed that this series conflicts with my work at redoing
how qapi union types are laid out (basically, I'm going from
backend->foo to backend->u.foo).  I'll list your series as a
prerequisite of mine.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v2 06/21] qemu-char: convert parallel backend to data-driven creation

[Eric Blake]

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

On 10/14/2015 09:54 AM, Paolo Bonzini wrote:
> Conversion to Error * brings better error messages; before:
> 
>     qemu-system-x86_64: -chardev id=serial,backend=parallel,path=vl.c: Failed to create chardev
> 
> After:
> 
>     qemu-system-x86_64: -chardev id=serial,backend=parallel,path=vl.c: not a parallel port: Inappropriate ioctl for device
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-char.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v2 07/21] qemu-char: convert pipe backend to data-driven creation

[Eric Blake]

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

On 10/14/2015 09:54 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-char.c | 37 +++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 16 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v2 14/21] qemu-char: convert braille backend to data-driven creation

[Eric Blake]

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

On 10/14/2015 09:54 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  backends/baum.c       | 16 +++++++++++-----
>  include/sysemu/char.h |  3 ---
>  qemu-char.c           |  4 +---
>  stubs/Makefile.objs   |  1 -
>  stubs/chr-baum-init.c |  7 -------
>  5 files changed, 12 insertions(+), 19 deletions(-)
>  delete mode 100644 stubs/chr-baum-init.c

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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