qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] chardev fixes
@ 2021-07-23 10:28 marcandre.lureau
  2021-07-23 10:28 ` [PATCH 1/4] chardev: fix qemu_chr_open_fd() being called with fd=-1 marcandre.lureau
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: marcandre.lureau @ 2021-07-23 10:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, berrange, Paolo Bonzini

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

Hi,

Two small fixes related to fd handling for "serial" and "file" backend and some
API comments and minor improvement.

Marc-André Lureau (4):
  chardev: fix qemu_chr_open_fd() being called with fd=-1
  chardev: fix qemu_chr_open_fd() with fd_in==fd_out
  chardev: remove needless class method
  chardev: add some comments about the class methods

 include/chardev/char.h | 34 +++++++++++++++++++++++++++++++++-
 chardev/char-fd.c      | 31 +++++++++++++++++++++++--------
 chardev/char-mux.c     |  6 ++----
 3 files changed, 58 insertions(+), 13 deletions(-)

-- 
2.32.0.264.g75ae10bc75




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] chardev: fix qemu_chr_open_fd() being called with fd=-1
  2021-07-23 10:28 [PATCH 0/4] chardev fixes marcandre.lureau
@ 2021-07-23 10:28 ` marcandre.lureau
  2021-08-03  8:40   ` Daniel P. Berrangé
  2021-07-23 10:28 ` [PATCH 2/4] chardev: fix qemu_chr_open_fd() with fd_in==fd_out marcandre.lureau
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: marcandre.lureau @ 2021-07-23 10:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, berrange, Paolo Bonzini

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

The "file" chardev may call qemu_chr_open_fd() with fd_in=-1. This may
cause invalid system calls, as the QIOChannel is assumed to be properly
initialized later on.

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

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 1cd62f2779..ee85a52c06 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -133,15 +133,19 @@ void qemu_chr_open_fd(Chardev *chr,
     FDChardev *s = FD_CHARDEV(chr);
     char *name;
 
-    s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
-    name = g_strdup_printf("chardev-file-in-%s", chr->label);
-    qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
-    g_free(name);
-    s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
-    name = g_strdup_printf("chardev-file-out-%s", chr->label);
-    qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
-    g_free(name);
-    qemu_set_nonblock(fd_out);
+    if (fd_in >= 0) {
+        s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
+        name = g_strdup_printf("chardev-file-in-%s", chr->label);
+        qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
+        g_free(name);
+    }
+    if (fd_out >= 0) {
+        s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
+        name = g_strdup_printf("chardev-file-out-%s", chr->label);
+        qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
+        g_free(name);
+        qemu_set_nonblock(fd_out);
+    }
 }
 
 static void char_fd_class_init(ObjectClass *oc, void *data)
-- 
2.32.0.264.g75ae10bc75



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] chardev: fix qemu_chr_open_fd() with fd_in==fd_out
  2021-07-23 10:28 [PATCH 0/4] chardev fixes marcandre.lureau
  2021-07-23 10:28 ` [PATCH 1/4] chardev: fix qemu_chr_open_fd() being called with fd=-1 marcandre.lureau
@ 2021-07-23 10:28 ` marcandre.lureau
  2021-08-03  8:44   ` Daniel P. Berrangé
  2021-07-23 10:28 ` [PATCH 3/4] chardev: remove needless class method marcandre.lureau
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: marcandre.lureau @ 2021-07-23 10:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, berrange, Paolo Bonzini

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

The "serial" chardev calls qemu_chr_open_fd() with the same fd. This
may lead to double-close as each QIOChannel owns the fd.

Instead, share the reference to the same QIOChannel.

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

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index ee85a52c06..32166182bf 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -139,13 +139,24 @@ void qemu_chr_open_fd(Chardev *chr,
         qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
         g_free(name);
     }
-    if (fd_out >= 0) {
+
+    if (fd_out < 0) {
+        return;
+    }
+
+    if (fd_out == fd_in) {
+        s->ioc_out = QIO_CHANNEL(object_ref(s->ioc_in));
+        name = g_strdup_printf("chardev-file-%s", chr->label);
+        qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
+        g_free(name);
+    } else {
         s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
         name = g_strdup_printf("chardev-file-out-%s", chr->label);
         qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
         g_free(name);
-        qemu_set_nonblock(fd_out);
     }
+
+    qemu_set_nonblock(fd_out);
 }
 
 static void char_fd_class_init(ObjectClass *oc, void *data)
-- 
2.32.0.264.g75ae10bc75



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] chardev: remove needless class method
  2021-07-23 10:28 [PATCH 0/4] chardev fixes marcandre.lureau
  2021-07-23 10:28 ` [PATCH 1/4] chardev: fix qemu_chr_open_fd() being called with fd=-1 marcandre.lureau
  2021-07-23 10:28 ` [PATCH 2/4] chardev: fix qemu_chr_open_fd() with fd_in==fd_out marcandre.lureau
@ 2021-07-23 10:28 ` marcandre.lureau
  2021-08-02 18:53   ` Philippe Mathieu-Daudé
  2021-08-03  8:45   ` Daniel P. Berrangé
  2021-07-23 10:28 ` [PATCH 4/4] chardev: add some comments about the class methods marcandre.lureau
  2021-08-02 18:49 ` [PATCH 0/4] chardev fixes Marc-André Lureau
  4 siblings, 2 replies; 12+ messages in thread
From: marcandre.lureau @ 2021-07-23 10:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, berrange, Paolo Bonzini

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

"chr_option_parsed" is only implemented by the "mux" chardev, we can
specialize the code there to avoid the needless generic class method.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char.h | 1 -
 chardev/char-mux.c     | 6 ++----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 7c0444f90d..589e7fe46d 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -273,7 +273,6 @@ struct ChardevClass {
     void (*chr_set_echo)(Chardev *chr, bool echo);
     void (*chr_set_fe_open)(Chardev *chr, int fe_open);
     void (*chr_be_event)(Chardev *s, QEMUChrEvent event);
-    void (*chr_options_parsed)(Chardev *chr);
 };
 
 Chardev *qemu_chardev_new(const char *id, const char *typename,
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 5baf419010..ada0c6866f 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -386,10 +386,9 @@ void suspend_mux_open(void)
 static int chardev_options_parsed_cb(Object *child, void *opaque)
 {
     Chardev *chr = (Chardev *)child;
-    ChardevClass *class = CHARDEV_GET_CLASS(chr);
 
-    if (!chr->be_open && class->chr_options_parsed) {
-        class->chr_options_parsed(chr);
+    if (!chr->be_open && CHARDEV_IS_MUX(chr)) {
+        open_muxes(chr);
     }
 
     return 0;
@@ -412,7 +411,6 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
     cc->chr_accept_input = mux_chr_accept_input;
     cc->chr_add_watch = mux_chr_add_watch;
     cc->chr_be_event = mux_chr_be_event;
-    cc->chr_options_parsed = open_muxes;
     cc->chr_update_read_handler = mux_chr_update_read_handlers;
 }
 
-- 
2.32.0.264.g75ae10bc75



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] chardev: add some comments about the class methods
  2021-07-23 10:28 [PATCH 0/4] chardev fixes marcandre.lureau
                   ` (2 preceding siblings ...)
  2021-07-23 10:28 ` [PATCH 3/4] chardev: remove needless class method marcandre.lureau
@ 2021-07-23 10:28 ` marcandre.lureau
  2021-08-03  8:46   ` Daniel P. Berrangé
  2021-08-02 18:49 ` [PATCH 0/4] chardev fixes Marc-André Lureau
  4 siblings, 1 reply; 12+ messages in thread
From: marcandre.lureau @ 2021-07-23 10:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, berrange, Paolo Bonzini

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

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

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 589e7fe46d..2e4c16f82f 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -254,24 +254,57 @@ struct ChardevClass {
 
     bool internal; /* TODO: eventually use TYPE_USER_CREATABLE */
     bool supports_yank;
+
+    /* parse command line options and populate QAPI @backend */
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
 
+    /* called after construction, open/starts the backend */
     void (*open)(Chardev *chr, ChardevBackend *backend,
                  bool *be_opened, Error **errp);
 
+    /* write buf to the backend */
     int (*chr_write)(Chardev *s, const uint8_t *buf, int len);
+
+    /*
+     * Read from the backend (blocking). A typical front-end will instead rely
+     * on char_can_read/chr_read being called when polling/looping.
+     */
     int (*chr_sync_read)(Chardev *s, const uint8_t *buf, int len);
+
+    /* create a watch on the backend */
     GSource *(*chr_add_watch)(Chardev *s, GIOCondition cond);
+
+    /* update the backend internal sources */
     void (*chr_update_read_handler)(Chardev *s);
+
+    /* send an ioctl to the backend */
     int (*chr_ioctl)(Chardev *s, int cmd, void *arg);
+
+    /* get ancillary-received fds during last read */
     int (*get_msgfds)(Chardev *s, int* fds, int num);
+
+    /* set ancillary fds to be sent with next write */
     int (*set_msgfds)(Chardev *s, int *fds, int num);
+
+    /* accept the given fd */
     int (*chr_add_client)(Chardev *chr, int fd);
+
+    /* wait for a connection */
     int (*chr_wait_connected)(Chardev *chr, Error **errp);
+
+    /* disconnect a connection */
     void (*chr_disconnect)(Chardev *chr);
+
+    /* called by frontend when it can read */
     void (*chr_accept_input)(Chardev *chr);
+
+    /* set terminal echo */
     void (*chr_set_echo)(Chardev *chr, bool echo);
+
+    /* notify the backend of frontend open state */
     void (*chr_set_fe_open)(Chardev *chr, int fe_open);
+
+    /* handle various events */
     void (*chr_be_event)(Chardev *s, QEMUChrEvent event);
 };
 
-- 
2.32.0.264.g75ae10bc75



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] chardev fixes
  2021-07-23 10:28 [PATCH 0/4] chardev fixes marcandre.lureau
                   ` (3 preceding siblings ...)
  2021-07-23 10:28 ` [PATCH 4/4] chardev: add some comments about the class methods marcandre.lureau
@ 2021-08-02 18:49 ` Marc-André Lureau
  4 siblings, 0 replies; 12+ messages in thread
From: Marc-André Lureau @ 2021-08-02 18:49 UTC (permalink / raw)
  To: QEMU; +Cc: Paolo Bonzini, Daniel P. Berrange

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

Hi

On Fri, Jul 23, 2021 at 2:29 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> Two small fixes related to fd handling for "serial" and "file" backend and
> some
> API comments and minor improvement.
>
> Marc-André Lureau (4):
>   chardev: fix qemu_chr_open_fd() being called with fd=-1
>   chardev: fix qemu_chr_open_fd() with fd_in==fd_out
>

I think I could queue those 2 patches for 6.1 as fixes. Daniel, care to
review?
thanks

  chardev: remove needless class method
>   chardev: add some comments about the class methods
>
>  include/chardev/char.h | 34 +++++++++++++++++++++++++++++++++-
>  chardev/char-fd.c      | 31 +++++++++++++++++++++++--------
>  chardev/char-mux.c     |  6 ++----
>  3 files changed, 58 insertions(+), 13 deletions(-)
>
> --
> 2.32.0.264.g75ae10bc75
>
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1613 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] chardev: remove needless class method
  2021-07-23 10:28 ` [PATCH 3/4] chardev: remove needless class method marcandre.lureau
@ 2021-08-02 18:53   ` Philippe Mathieu-Daudé
  2021-08-03  8:45   ` Daniel P. Berrangé
  1 sibling, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-02 18:53 UTC (permalink / raw)
  To: marcandre.lureau, qemu-devel; +Cc: Paolo Bonzini, berrange

On 7/23/21 12:28 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> "chr_option_parsed" is only implemented by the "mux" chardev, we can
> specialize the code there to avoid the needless generic class method.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/chardev/char.h | 1 -
>  chardev/char-mux.c     | 6 ++----
>  2 files changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] chardev: fix qemu_chr_open_fd() being called with fd=-1
  2021-07-23 10:28 ` [PATCH 1/4] chardev: fix qemu_chr_open_fd() being called with fd=-1 marcandre.lureau
@ 2021-08-03  8:40   ` Daniel P. Berrangé
  2021-08-04 11:11     ` Marc-André Lureau
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2021-08-03  8:40 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: Paolo Bonzini, qemu-devel

On Fri, Jul 23, 2021 at 02:28:22PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The "file" chardev may call qemu_chr_open_fd() with fd_in=-1. This may
> cause invalid system calls, as the QIOChannel is assumed to be properly
> initialized later on.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char-fd.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index 1cd62f2779..ee85a52c06 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -133,15 +133,19 @@ void qemu_chr_open_fd(Chardev *chr,
>      FDChardev *s = FD_CHARDEV(chr);
>      char *name;
>  
> -    s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
> -    name = g_strdup_printf("chardev-file-in-%s", chr->label);
> -    qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
> -    g_free(name);
> -    s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
> -    name = g_strdup_printf("chardev-file-out-%s", chr->label);
> -    qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
> -    g_free(name);
> -    qemu_set_nonblock(fd_out);
> +    if (fd_in >= 0) {
> +        s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
> +        name = g_strdup_printf("chardev-file-in-%s", chr->label);
> +        qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
> +        g_free(name);
> +    }
> +    if (fd_out >= 0) {
> +        s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
> +        name = g_strdup_printf("chardev-file-out-%s", chr->label);
> +        qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
> +        g_free(name);
> +        qemu_set_nonblock(fd_out);
> +    }

Other code in this file assumes ioc_out is non-NULL, so this looks
like an incomplete fix.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] chardev: fix qemu_chr_open_fd() with fd_in==fd_out
  2021-07-23 10:28 ` [PATCH 2/4] chardev: fix qemu_chr_open_fd() with fd_in==fd_out marcandre.lureau
@ 2021-08-03  8:44   ` Daniel P. Berrangé
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2021-08-03  8:44 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: Paolo Bonzini, qemu-devel

On Fri, Jul 23, 2021 at 02:28:23PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The "serial" chardev calls qemu_chr_open_fd() with the same fd. This
> may lead to double-close as each QIOChannel owns the fd.
> 
> Instead, share the reference to the same QIOChannel.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char-fd.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index ee85a52c06..32166182bf 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -139,13 +139,24 @@ void qemu_chr_open_fd(Chardev *chr,
>          qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
>          g_free(name);
>      }
> -    if (fd_out >= 0) {
> +
> +    if (fd_out < 0) {
> +        return;
> +    }
> +
> +    if (fd_out == fd_in) {
> +        s->ioc_out = QIO_CHANNEL(object_ref(s->ioc_in));
> +        name = g_strdup_printf("chardev-file-%s", chr->label);
> +        qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
> +        g_free(name);

This is overwriting the name set a few lines earlier. I think the
code ought to be refactor to eliminate this duplication.

ie

  if (fd_out == fd_in) {
    s->ioc_out = s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
    ....
  } else {
    s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
    ...
    s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
    ...
  }

> +    } else {
>          s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
>          name = g_strdup_printf("chardev-file-out-%s", chr->label);
>          qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
>          g_free(name);
> -        qemu_set_nonblock(fd_out);
>      }
> +
> +    qemu_set_nonblock(fd_out);
>  }
>  
>  static void char_fd_class_init(ObjectClass *oc, void *data)
> -- 
> 2.32.0.264.g75ae10bc75
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] chardev: remove needless class method
  2021-07-23 10:28 ` [PATCH 3/4] chardev: remove needless class method marcandre.lureau
  2021-08-02 18:53   ` Philippe Mathieu-Daudé
@ 2021-08-03  8:45   ` Daniel P. Berrangé
  1 sibling, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2021-08-03  8:45 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: Paolo Bonzini, qemu-devel

On Fri, Jul 23, 2021 at 02:28:24PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> "chr_option_parsed" is only implemented by the "mux" chardev, we can
> specialize the code there to avoid the needless generic class method.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/chardev/char.h | 1 -
>  chardev/char-mux.c     | 6 ++----
>  2 files changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] chardev: add some comments about the class methods
  2021-07-23 10:28 ` [PATCH 4/4] chardev: add some comments about the class methods marcandre.lureau
@ 2021-08-03  8:46   ` Daniel P. Berrangé
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2021-08-03  8:46 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: Paolo Bonzini, qemu-devel

On Fri, Jul 23, 2021 at 02:28:25PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/chardev/char.h | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 589e7fe46d..2e4c16f82f 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -254,24 +254,57 @@ struct ChardevClass {
>  
>      bool internal; /* TODO: eventually use TYPE_USER_CREATABLE */
>      bool supports_yank;
> +
> +    /* parse command line options and populate QAPI @backend */
>      void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>  
> +    /* called after construction, open/starts the backend */
>      void (*open)(Chardev *chr, ChardevBackend *backend,
>                   bool *be_opened, Error **errp);
>  
> +    /* write buf to the backend */
>      int (*chr_write)(Chardev *s, const uint8_t *buf, int len);
> +
> +    /*
> +     * Read from the backend (blocking). A typical front-end will instead rely
> +     * on char_can_read/chr_read being called when polling/looping.
> +     */

chr_can_read

>      int (*chr_sync_read)(Chardev *s, const uint8_t *buf, int len);
> +
> +    /* create a watch on the backend */
>      GSource *(*chr_add_watch)(Chardev *s, GIOCondition cond);
> +
> +    /* update the backend internal sources */
>      void (*chr_update_read_handler)(Chardev *s);
> +
> +    /* send an ioctl to the backend */
>      int (*chr_ioctl)(Chardev *s, int cmd, void *arg);
> +
> +    /* get ancillary-received fds during last read */
>      int (*get_msgfds)(Chardev *s, int* fds, int num);
> +
> +    /* set ancillary fds to be sent with next write */
>      int (*set_msgfds)(Chardev *s, int *fds, int num);
> +
> +    /* accept the given fd */
>      int (*chr_add_client)(Chardev *chr, int fd);
> +
> +    /* wait for a connection */
>      int (*chr_wait_connected)(Chardev *chr, Error **errp);
> +
> +    /* disconnect a connection */
>      void (*chr_disconnect)(Chardev *chr);
> +
> +    /* called by frontend when it can read */
>      void (*chr_accept_input)(Chardev *chr);
> +
> +    /* set terminal echo */
>      void (*chr_set_echo)(Chardev *chr, bool echo);
> +
> +    /* notify the backend of frontend open state */
>      void (*chr_set_fe_open)(Chardev *chr, int fe_open);
> +
> +    /* handle various events */
>      void (*chr_be_event)(Chardev *s, QEMUChrEvent event);
>  };

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] chardev: fix qemu_chr_open_fd() being called with fd=-1
  2021-08-03  8:40   ` Daniel P. Berrangé
@ 2021-08-04 11:11     ` Marc-André Lureau
  0 siblings, 0 replies; 12+ messages in thread
From: Marc-André Lureau @ 2021-08-04 11:11 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: Paolo Bonzini, QEMU

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

Hi

On Tue, Aug 3, 2021 at 12:41 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, Jul 23, 2021 at 02:28:22PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > The "file" chardev may call qemu_chr_open_fd() with fd_in=-1. This may
> > cause invalid system calls, as the QIOChannel is assumed to be properly
> > initialized later on.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  chardev/char-fd.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> > index 1cd62f2779..ee85a52c06 100644
> > --- a/chardev/char-fd.c
> > +++ b/chardev/char-fd.c
> > @@ -133,15 +133,19 @@ void qemu_chr_open_fd(Chardev *chr,
> >      FDChardev *s = FD_CHARDEV(chr);
> >      char *name;
> >
> > -    s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
> > -    name = g_strdup_printf("chardev-file-in-%s", chr->label);
> > -    qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
> > -    g_free(name);
> > -    s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
> > -    name = g_strdup_printf("chardev-file-out-%s", chr->label);
> > -    qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
> > -    g_free(name);
> > -    qemu_set_nonblock(fd_out);
> > +    if (fd_in >= 0) {
> > +        s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
> > +        name = g_strdup_printf("chardev-file-in-%s", chr->label);
> > +        qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
> > +        g_free(name);
> > +    }
> > +    if (fd_out >= 0) {
> > +        s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
> > +        name = g_strdup_printf("chardev-file-out-%s", chr->label);
> > +        qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
> > +        g_free(name);
> > +        qemu_set_nonblock(fd_out);
> > +    }
>
> Other code in this file assumes ioc_out is non-NULL, so this looks
> like an incomplete fix.
>
>
Right, I am adding a patch to correct the source watch creation, and fixing
this one handling !ioc_out condition.

thanks

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3260 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-08-04 11:13 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 10:28 [PATCH 0/4] chardev fixes marcandre.lureau
2021-07-23 10:28 ` [PATCH 1/4] chardev: fix qemu_chr_open_fd() being called with fd=-1 marcandre.lureau
2021-08-03  8:40   ` Daniel P. Berrangé
2021-08-04 11:11     ` Marc-André Lureau
2021-07-23 10:28 ` [PATCH 2/4] chardev: fix qemu_chr_open_fd() with fd_in==fd_out marcandre.lureau
2021-08-03  8:44   ` Daniel P. Berrangé
2021-07-23 10:28 ` [PATCH 3/4] chardev: remove needless class method marcandre.lureau
2021-08-02 18:53   ` Philippe Mathieu-Daudé
2021-08-03  8:45   ` Daniel P. Berrangé
2021-07-23 10:28 ` [PATCH 4/4] chardev: add some comments about the class methods marcandre.lureau
2021-08-03  8:46   ` Daniel P. Berrangé
2021-08-02 18:49 ` [PATCH 0/4] chardev fixes Marc-André Lureau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).