[Qemu-devel] [PATCH v3] chardev: Allow for pty path passing.

Paulo Neves posted 1 patch 5 years, 3 months ago
Test docker-mingw@fedora failed
Test asan passed
Test checkpatch passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1548509635-15776-1-git-send-email-ptsneves@gmail.com
Maintainers: Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
chardev/char-pty.c | 41 ++++++++++++++++++++++++++++++++++++++++-
chardev/char.c     |  6 +++++-
qapi/char.json     |  4 ++--
3 files changed, 47 insertions(+), 4 deletions(-)
[Qemu-devel] [PATCH v3] chardev: Allow for pty path passing.
Posted by Paulo Neves 5 years, 3 months ago
If a user requires a virtual serial device like provided
by the pty char device, the user needs to accept the
returned device name. This makes the program need to
have smarts to parse or communicate with qemu to get the
pty device.
With this patch the program can pass the path where
a symlink to the pty device will be, removing the
need for 2 way communication or smarts.

Signed-off-by: Paulo Neves <ptsneves@gmail.com>
---
 chardev/char-pty.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 chardev/char.c     |  6 +++++-
 qapi/char.json     |  4 ++--
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index f681d63..5c312ce 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -28,12 +28,14 @@
 #include "io/channel-file.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
+#include "qemu/option.h"
 
 #include "chardev/char-io.h"
 
 typedef struct {
     Chardev parent;
     QIOChannel *ioc;
+    char *link_name;
     int read_bytes;
 
     /* Protected by the Chardev chr_write_lock.  */
@@ -234,6 +236,12 @@ static void char_pty_finalize(Object *obj)
     qemu_mutex_lock(&chr->chr_write_lock);
     pty_chr_state(chr, 0);
     object_unref(OBJECT(s->ioc));
+
+    if (s->link_name) {
+        unlink(s->link_name);
+        g_free(s->link_name);
+    }
+
     pty_chr_timer_cancel(s);
     qemu_mutex_unlock(&chr->chr_write_lock);
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -244,8 +252,9 @@ static void char_pty_open(Chardev *chr,
                           bool *be_opened,
                           Error **errp)
 {
+    ChardevHostdev *opts = backend->u.pty.data;
     PtyChardev *s;
-    int master_fd, slave_fd;
+    int master_fd, slave_fd, symlink_ret;
     char pty_name[PATH_MAX];
     char *name;
 
@@ -256,6 +265,17 @@ static void char_pty_open(Chardev *chr,
     }
 
     close(slave_fd);
+
+    s = PTY_CHARDEV(chr);
+    s->link_name = g_strdup(opts->device);
+    symlink_ret = symlink(pty_name, s->link_name);
+
+    if (symlink_ret < 0) {
+        close(master_fd);
+        error_setg_errno(errp, errno, "Failed to create symlink to PTY");
+        return;
+    }
+
     qemu_set_nonblock(master_fd);
 
     chr->filename = g_strdup_printf("pty:%s", pty_name);
@@ -271,6 +291,24 @@ static void char_pty_open(Chardev *chr,
     *be_opened = false;
 }
 
+static void char_pty_parse(QemuOpts *opts, ChardevBackend *backend,
+                                Error **errp)
+{
+    const char *symlink_path = qemu_opt_get(opts, "path");
+    if (symlink_path == NULL) {
+        error_setg(errp, "chardev: pty symlink: no device path given");
+        return;
+
+    }
+
+    ChardevHostdev *dev;
+
+    backend->type = CHARDEV_BACKEND_KIND_PTY;
+    dev = backend->u.pipe.data = g_new0(ChardevHostdev, 1);
+    qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(dev));
+    dev->device = g_strdup(symlink_path);
+}
+
 static void char_pty_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
@@ -279,6 +317,7 @@ static void char_pty_class_init(ObjectClass *oc, void *data)
     cc->chr_write = char_pty_chr_write;
     cc->chr_update_read_handler = pty_chr_update_read_handler;
     cc->chr_add_watch = pty_chr_add_watch;
+    cc->parse = char_pty_parse;
 }
 
 static const TypeInfo char_pty_type_info = {
diff --git a/chardev/char.c b/chardev/char.c
index ccba36b..43fce4a 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -373,7 +373,6 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
     }
 
     if (strcmp(filename, "null")    == 0 ||
-        strcmp(filename, "pty")     == 0 ||
         strcmp(filename, "msmouse") == 0 ||
         strcmp(filename, "wctablet") == 0 ||
         strcmp(filename, "braille") == 0 ||
@@ -418,6 +417,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
         qemu_opt_set(opts, "path", p, &error_abort);
         return opts;
     }
+    if (strstart(filename, "pty:", &p)) {
+        qemu_opt_set(opts, "backend", "pty", &error_abort);
+        qemu_opt_set(opts, "path", p, &error_abort);
+        return opts;
+    }
     if (strstart(filename, "tcp:", &p) ||
         strstart(filename, "telnet:", &p) ||
         strstart(filename, "tn3270:", &p) ||
diff --git a/qapi/char.json b/qapi/char.json
index 77ed847..88c62bd 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -229,7 +229,7 @@
 ##
 # @ChardevHostdev:
 #
-# Configuration info for device and pipe chardevs.
+# Configuration info for device, pty and pipe chardevs.
 #
 # @device: The name of the special file for the device,
 #          i.e. /dev/ttyS0 on Unix or COM1: on Windows
@@ -395,7 +395,7 @@
             'pipe': 'ChardevHostdev',
             'socket': 'ChardevSocket',
             'udp': 'ChardevUdp',
-            'pty': 'ChardevCommon',
+            'pty': 'ChardevHostdev',
             'null': 'ChardevCommon',
             'mux': 'ChardevMux',
             'msmouse': 'ChardevCommon',
-- 
2.7.4


Re: [Qemu-devel] [PATCH v3] chardev: Allow for pty path passing.
Posted by no-reply@patchew.org 5 years, 2 months ago
Patchew URL: https://patchew.org/QEMU/1548509635-15776-1-git-send-email-ptsneves@gmail.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/1548509635-15776-1-git-send-email-ptsneves@gmail.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [Qemu-devel] [PATCH v3] chardev: Allow for pty path passing.
Posted by Marc-André Lureau 5 years, 2 months ago
Hi

On Sat, Jan 26, 2019 at 2:34 PM Paulo Neves <ptsneves@gmail.com> wrote:
>
> If a user requires a virtual serial device like provided
> by the pty char device, the user needs to accept the
> returned device name. This makes the program need to
> have smarts to parse or communicate with qemu to get the
> pty device.
> With this patch the program can pass the path where
> a symlink to the pty device will be, removing the
> need for 2 way communication or smarts.
>
> Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> ---
>  chardev/char-pty.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  chardev/char.c     |  6 +++++-
>  qapi/char.json     |  4 ++--
>  3 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index f681d63..5c312ce 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -28,12 +28,14 @@
>  #include "io/channel-file.h"
>  #include "qemu/sockets.h"
>  #include "qemu/error-report.h"
> +#include "qemu/option.h"
>
>  #include "chardev/char-io.h"
>
>  typedef struct {
>      Chardev parent;
>      QIOChannel *ioc;
> +    char *link_name;
>      int read_bytes;
>
>      /* Protected by the Chardev chr_write_lock.  */
> @@ -234,6 +236,12 @@ static void char_pty_finalize(Object *obj)
>      qemu_mutex_lock(&chr->chr_write_lock);
>      pty_chr_state(chr, 0);
>      object_unref(OBJECT(s->ioc));
> +
> +    if (s->link_name) {
> +        unlink(s->link_name);
> +        g_free(s->link_name);
> +    }
> +
>      pty_chr_timer_cancel(s);
>      qemu_mutex_unlock(&chr->chr_write_lock);
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> @@ -244,8 +252,9 @@ static void char_pty_open(Chardev *chr,
>                            bool *be_opened,
>                            Error **errp)
>  {
> +    ChardevHostdev *opts = backend->u.pty.data;
>      PtyChardev *s;
> -    int master_fd, slave_fd;
> +    int master_fd, slave_fd, symlink_ret;
>      char pty_name[PATH_MAX];
>      char *name;
>
> @@ -256,6 +265,17 @@ static void char_pty_open(Chardev *chr,
>      }
>
>      close(slave_fd);
> +
> +    s = PTY_CHARDEV(chr);
> +    s->link_name = g_strdup(opts->device);
> +    symlink_ret = symlink(pty_name, s->link_name);
> +
> +    if (symlink_ret < 0) {
> +        close(master_fd);
> +        error_setg_errno(errp, errno, "Failed to create symlink to PTY");

Leaving s->link_name will lead finalize() to unlink() a file that it
hasn't created.

suggest to symlink() with opts->device.

Please also drop the extra symlink_ret variable.

> +        return;
> +    }
> +
>      qemu_set_nonblock(master_fd);
>
>      chr->filename = g_strdup_printf("pty:%s", pty_name);
> @@ -271,6 +291,24 @@ static void char_pty_open(Chardev *chr,
>      *be_opened = false;
>  }
>
> +static void char_pty_parse(QemuOpts *opts, ChardevBackend *backend,
> +                                Error **errp)
> +{
> +    const char *symlink_path = qemu_opt_get(opts, "path");
> +    if (symlink_path == NULL) {
> +        error_setg(errp, "chardev: pty symlink: no device path given");
> +        return;
> +
> +    }
> +
> +    ChardevHostdev *dev;
> +

Please keep declarations groupped, and add an empty line after
declarations. This generally looks more consistent and easier to read.

> +    backend->type = CHARDEV_BACKEND_KIND_PTY;
> +    dev = backend->u.pipe.data = g_new0(ChardevHostdev, 1);
> +    qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(dev));
> +    dev->device = g_strdup(symlink_path);
> +}
> +
>  static void char_pty_class_init(ObjectClass *oc, void *data)
>  {
>      ChardevClass *cc = CHARDEV_CLASS(oc);
> @@ -279,6 +317,7 @@ static void char_pty_class_init(ObjectClass *oc, void *data)
>      cc->chr_write = char_pty_chr_write;
>      cc->chr_update_read_handler = pty_chr_update_read_handler;
>      cc->chr_add_watch = pty_chr_add_watch;
> +    cc->parse = char_pty_parse;
>  }
>
>  static const TypeInfo char_pty_type_info = {
> diff --git a/chardev/char.c b/chardev/char.c
> index ccba36b..43fce4a 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -373,7 +373,6 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>      }
>
>      if (strcmp(filename, "null")    == 0 ||
> -        strcmp(filename, "pty")     == 0 ||

This breaks existing "pty" filename. Please fix.

>          strcmp(filename, "msmouse") == 0 ||
>          strcmp(filename, "wctablet") == 0 ||
>          strcmp(filename, "braille") == 0 ||
> @@ -418,6 +417,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>          qemu_opt_set(opts, "path", p, &error_abort);
>          return opts;
>      }
> +    if (strstart(filename, "pty:", &p)) {
> +        qemu_opt_set(opts, "backend", "pty", &error_abort);
> +        qemu_opt_set(opts, "path", p, &error_abort);
> +        return opts;
> +    }
>      if (strstart(filename, "tcp:", &p) ||
>          strstart(filename, "telnet:", &p) ||
>          strstart(filename, "tn3270:", &p) ||
> diff --git a/qapi/char.json b/qapi/char.json
> index 77ed847..88c62bd 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -229,7 +229,7 @@
>  ##
>  # @ChardevHostdev:
>  #
> -# Configuration info for device and pipe chardevs.
> +# Configuration info for device, pty and pipe chardevs.
>  #
>  # @device: The name of the special file for the device,
>  #          i.e. /dev/ttyS0 on Unix or COM1: on Windows
> @@ -395,7 +395,7 @@
>              'pipe': 'ChardevHostdev',
>              'socket': 'ChardevSocket',
>              'udp': 'ChardevUdp',
> -            'pty': 'ChardevCommon',
> +            'pty': 'ChardevHostdev',
>              'null': 'ChardevCommon',
>              'mux': 'ChardevMux',
>              'msmouse': 'ChardevCommon',
> --
> 2.7.4
>
>

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3] chardev: Allow for pty path passing.
Posted by Daniel P. Berrangé 5 years, 2 months ago
On Sat, Jan 26, 2019 at 02:33:55PM +0100, Paulo Neves wrote:
> If a user requires a virtual serial device like provided
> by the pty char device, the user needs to accept the
> returned device name. This makes the program need to
> have smarts to parse or communicate with qemu to get the
> pty device.
> With this patch the program can pass the path where
> a symlink to the pty device will be, removing the
> need for 2 way communication or smarts.
> 
> Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> ---
>  chardev/char-pty.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  chardev/char.c     |  6 +++++-
>  qapi/char.json     |  4 ++--
>  3 files changed, 47 insertions(+), 4 deletions(-)

qemu-options.hx needs updating to document the new syntax too


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 :|