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

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

[Paulo Neves]

Hello all.
I am trying to get qemu to spawn a pty terminal in a location
which I control without needing to retrieve information of the PTY device
a-posteriori from qemu. To this end i inspired myself in the functionality
of socat, which allows PTY terminals to be spawned in a location
specified by a command line argument. The way socat does is a
bit cheating: It requests the PTY terminal the same way that qemu
does, through openpty, and creates a symlink between the path
specified by the user and the kernel given virtual terminal device.

In the following patch i was able to implement this functionality
successfully and it works as intended. The problem is that for some
reason when I send Ctrl-a-x through the pty the finalize method of
the pty char dev does not get called, thus not removing the symlink.
The finalize only is called when there is an error in the creation of
the symlink like for example when the symlink already exists.

This leads to a dangling symlink when qemu closes and the PTY is released.
Is there a better way to guarantee that my unlink of the symlink is
called when qemu closes? Am i not familiar with qemu code development
so if i am doing something terrible please let me know.

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

[Paulo Neves]

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.
---
 chardev/char-pty.c | 38 ++++++++++++++++++++++++++++++++++++--
 chardev/char.c     |  6 +++++-
 qapi/char.json     |  4 ++--
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 761ae6d..b465263 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -38,6 +38,7 @@
 typedef struct {
     Chardev parent;
     QIOChannel *ioc;
+    char *link_name;
     int read_bytes;
 
     /* Protected by the Chardev chr_write_lock.  */
@@ -231,6 +232,11 @@ 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);
+    }
+
     if (s->timer_tag) {
         g_source_remove(s->timer_tag);
         s->timer_tag = 0;
@@ -244,8 +250,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,13 +263,23 @@ static void char_pty_open(Chardev *chr,
     }
 
     close(slave_fd);
+
+    s = PTY_CHARDEV(chr);
+    s->link_name = 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);
     error_report("char device redirected to %s (label %s)",
                  pty_name, chr->label);
 
-    s = PTY_CHARDEV(chr);
     s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
     name = g_strdup_printf("chardev-pty-%s", chr->label);
     qio_channel_set_name(QIO_CHANNEL(s->ioc), name);
@@ -271,6 +288,22 @@ 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 +312,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 5d52cd5..e4c5371 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -357,7 +357,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 ||
@@ -402,6 +401,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 6de0f29..dae4231 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -224,7 +224,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
@@ -380,7 +380,7 @@
                                        'pipe'   : 'ChardevHostdev',
                                        'socket' : 'ChardevSocket',
                                        'udp'    : 'ChardevUdp',
-                                       'pty'    : 'ChardevCommon',
+                                       'pty'    : 'ChardevHostdev',
                                        'null'   : 'ChardevCommon',
                                        'mux'    : 'ChardevMux',
                                        'msmouse': 'ChardevCommon',
-- 
2.7.4

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

[Marc-André Lureau]

Hi

On Sat, Dec 29, 2018 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.

This seems reasonable. Your patch doesn't apply on master, can you rebase it?


> ---
>  chardev/char-pty.c | 38 ++++++++++++++++++++++++++++++++++++--
>  chardev/char.c     |  6 +++++-
>  qapi/char.json     |  4 ++--
>  3 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index 761ae6d..b465263 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -38,6 +38,7 @@
>  typedef struct {
>      Chardev parent;
>      QIOChannel *ioc;
> +    char *link_name;
>      int read_bytes;
>
>      /* Protected by the Chardev chr_write_lock.  */
> @@ -231,6 +232,11 @@ 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);
> +    }
> +

I suspect a g_free(s->link_name) is missing

>      if (s->timer_tag) {
>          g_source_remove(s->timer_tag);
>          s->timer_tag = 0;
> @@ -244,8 +250,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,13 +263,23 @@ static void char_pty_open(Chardev *chr,
>      }
>
>      close(slave_fd);
> +
> +    s = PTY_CHARDEV(chr)
> +    s->link_name = opts->device;

You should g_strdup() it

> +    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);
>      error_report("char device redirected to %s (label %s)",
>                   pty_name, chr->label);
>
> -    s = PTY_CHARDEV(chr);
>      s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
>      name = g_strdup_printf("chardev-pty-%s", chr->label);
>      qio_channel_set_name(QIO_CHANNEL(s->ioc), name);
> @@ -271,6 +288,22 @@ 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 +312,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 5d52cd5..e4c5371 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -357,7 +357,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 ||
> @@ -402,6 +401,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 6de0f29..dae4231 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -224,7 +224,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
> @@ -380,7 +380,7 @@
>                                         'pipe'   : 'ChardevHostdev',
>                                         'socket' : 'ChardevSocket',
>                                         'udp'    : 'ChardevUdp',
> -                                       'pty'    : 'ChardevCommon',
> +                                       'pty'    : 'ChardevHostdev',

Better to create a new ChardevPty instead (ChardevHostdev is for host
device, here it is not)

>                                         'null'   : 'ChardevCommon',
>                                         'mux'    : 'ChardevMux',
>                                         'msmouse': 'ChardevCommon',
> --
> 2.7.4
>
>


-- 
Marc-André Lureau

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

[Eric Blake]

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

On 1/2/19 2:23 AM, Marc-André Lureau wrote:
> Hi
> 
> On Sat, Dec 29, 2018 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.
> 
> This seems reasonable. Your patch doesn't apply on master, can you rebase it?
> 

>> +++ b/qapi/char.json
>> @@ -224,7 +224,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
>> @@ -380,7 +380,7 @@
>>                                         'pipe'   : 'ChardevHostdev',
>>                                         'socket' : 'ChardevSocket',
>>                                         'udp'    : 'ChardevUdp',
>> -                                       'pty'    : 'ChardevCommon',
>> +                                       'pty'    : 'ChardevHostdev',
> 
> Better to create a new ChardevPty instead (ChardevHostdev is for host
> device, here it is not)

A new type with identical member names is indistinguishable in
introspection.  You could even rename s/ChardevHostdev/ChardevDevice/
for use by both 'pipe' and 'pty', since all the more ChardevHostdev did
over ChardevCommon was add a 'device' member (since renaming types is
transparent to introspection).

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


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

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

[Paulo Neves]

On Wed, Jan 2, 2019 at 9:24 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Sat, Dec 29, 2018 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.
>
> This seems reasonable. Your patch doesn't apply on master, can you rebase it?
Will do so. A patch V2 was sent with the rebase and the sign-off which
i had forgotten.
>
> > ---
> >  chardev/char-pty.c | 38 ++++++++++++++++++++++++++++++++++++--
> >  chardev/char.c     |  6 +++++-
> >  qapi/char.json     |  4 ++--
> >  3 files changed, 43 insertions(+), 5 deletions(-)
> >
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index 761ae6d..b465263 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -38,6 +38,7 @@
> >  typedef struct {
> >      Chardev parent;
> >      QIOChannel *ioc;
> > +    char *link_name;
> >      int read_bytes;
> >
> >      /* Protected by the Chardev chr_write_lock.  */
> > @@ -231,6 +232,11 @@ 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);
> > +    }
> > +
>
> I suspect a g_free(s->link_name) is missing
I am not sure we own the opts-device memory, so in the new patch i
indeed do g_free, but I free what I strdup'ed.
Did i misinterpret your review, or do you mean that I should g_free
opts->device?
>
> >      if (s->timer_tag) {
> >          g_source_remove(s->timer_tag);
> >          s->timer_tag = 0;
> > @@ -244,8 +250,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,13 +263,23 @@ static void char_pty_open(Chardev *chr,
> >      }
> >
> >      close(slave_fd);
> > +
> > +    s = PTY_CHARDEV(chr)
> > +    s->link_name = opts->device;
>
> You should g_strdup() it
Done
>
> > +    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);
> >      error_report("char device redirected to %s (label %s)",
> >                   pty_name, chr->label);
> >
> > -    s = PTY_CHARDEV(chr);
> >      s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
> >      name = g_strdup_printf("chardev-pty-%s", chr->label);
> >      qio_channel_set_name(QIO_CHANNEL(s->ioc), name);
> > @@ -271,6 +288,22 @@ 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 +312,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 5d52cd5..e4c5371 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -357,7 +357,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 ||
> > @@ -402,6 +401,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 6de0f29..dae4231 100644
> > --- a/qapi/char.json
> > +++ b/qapi/char.json
> > @@ -224,7 +224,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
> > @@ -380,7 +380,7 @@
> >                                         'pipe'   : 'ChardevHostdev',
> >                                         'socket' : 'ChardevSocket',
> >                                         'udp'    : 'ChardevUdp',
> > -                                       'pty'    : 'ChardevCommon',
> > +                                       'pty'    : 'ChardevHostdev',
>
> Better to create a new ChardevPty instead (ChardevHostdev is for host
> device, here it is not)
I did not completely understand what is the advantage here as, like a
pipe the pty is provided by the host machine indeed.
I will modify the patch if this is indeed required. Maybe Eric can
further clarify if indeed due to introspection equivalence the change
is not necessary.
>
> >                                         'null'   : 'ChardevCommon',
> >                                         'mux'    : 'ChardevMux',
> >                                         'msmouse': 'ChardevCommon',
> > --
> > 2.7.4
> >
> >
>
>
> --
> Marc-André Lureau

Paulo Neves