* [Qemu-devel] [PATCH] chardev: Allow for pty path passing. @ 2018-12-29 10:15 Paulo Neves 2018-12-29 10:15 ` Paulo Neves 0 siblings, 1 reply; 5+ messages in thread From: Paulo Neves @ 2018-12-29 10:15 UTC (permalink / raw) To: qemu-devel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH] chardev: Allow for pty path passing. 2018-12-29 10:15 [Qemu-devel] [PATCH] chardev: Allow for pty path passing Paulo Neves @ 2018-12-29 10:15 ` Paulo Neves 2019-01-02 8:23 ` Marc-André Lureau 0 siblings, 1 reply; 5+ messages in thread From: Paulo Neves @ 2018-12-29 10:15 UTC (permalink / raw) To: qemu-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] chardev: Allow for pty path passing. 2018-12-29 10:15 ` Paulo Neves @ 2019-01-02 8:23 ` Marc-André Lureau 2019-01-03 20:18 ` Eric Blake 2019-01-03 21:14 ` Paulo Neves 0 siblings, 2 replies; 5+ messages in thread From: Marc-André Lureau @ 2019-01-02 8:23 UTC (permalink / raw) To: Paulo Neves; +Cc: QEMU 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] chardev: Allow for pty path passing. 2019-01-02 8:23 ` Marc-André Lureau @ 2019-01-03 20:18 ` Eric Blake 2019-01-03 21:14 ` Paulo Neves 1 sibling, 0 replies; 5+ messages in thread From: Eric Blake @ 2019-01-03 20:18 UTC (permalink / raw) To: Marc-André Lureau, Paulo Neves; +Cc: QEMU [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] chardev: Allow for pty path passing. 2019-01-02 8:23 ` Marc-André Lureau 2019-01-03 20:18 ` Eric Blake @ 2019-01-03 21:14 ` Paulo Neves 1 sibling, 0 replies; 5+ messages in thread From: Paulo Neves @ 2019-01-03 21:14 UTC (permalink / raw) To: Marc-André Lureau; +Cc: QEMU, Eric Blake 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-03 21:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-29 10:15 [Qemu-devel] [PATCH] chardev: Allow for pty path passing Paulo Neves 2018-12-29 10:15 ` Paulo Neves 2019-01-02 8:23 ` Marc-André Lureau 2019-01-03 20:18 ` Eric Blake 2019-01-03 21:14 ` Paulo Neves
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).