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