After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 320128 - [patch] please provide a way to keep fds open with "vte_terminal_fork_pty"
[patch] please provide a way to keep fds open with "vte_terminal_fork_pty"
Status: RESOLVED OBSOLETE
Product: vte
Classification: Core
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: VTE Maintainers
VTE Maintainers
Depends on: 585841
Blocks: 81560 vte1.0
 
 
Reported: 2005-10-28 21:03 UTC by Michael Vogt
Modified: 2011-05-04 11:40 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
patch that implements a VTE_KEEP_FD_OPEN enviroment (1.53 KB, patch)
2005-10-28 21:04 UTC, Michael Vogt
rejected Details | Review
A patch to add a new variant of forkpty() that doesn't close the currently open file descriptors. (9.09 KB, patch)
2008-10-19 21:09 UTC, Daniel Burrows
needs-work Details | Review
[PATCH] Bug 320128 – [patch] please provide a way to keep fds open with "vte_terminal_fork_pty" (18.38 KB, patch)
2008-10-19 22:33 UTC, Christian Persch
none Details | Review

Description Michael Vogt 2005-10-28 21:03:38 UTC
I would like to have a way to prevent vte from closing certain file
descriptiors. I want to use it in my synaptic<->apt interaction where I spawn a
child that runs in a terminal but have a open status-fd that reports back to the
parent process. I'm pretty sure it is useful for other applications as well. 

I attached a patch that implements this feature with a ""VTE_PTY_KEEP_FD"
enviroment that is passed to the child. I'm not sure if that's the best way to
implment it but it was good enough for me to test and prototype :)
Comment 1 Michael Vogt 2005-10-28 21:04:46 UTC
Created attachment 54017 [details] [review]
patch that implements a VTE_KEEP_FD_OPEN enviroment
Comment 2 Olav Vitters 2005-12-29 16:18:35 UTC
Ehr.. so this is a vte specific hack and other terminals will not have it? What about using expect or something?
Comment 3 Michael Vogt 2005-12-29 19:43:08 UTC
This patch is for libvte and I think it's fine if other terminals don't have it. 

My use-case is to fork a pty and display stuff in the terminal widget and have the child that runs in the terminal report it's actions in a machine readable way back to the parent. This is done with a pipe that is shared between parent and child. The current vte closes all file descriptors when the child is forked and this obviously closes my pipe as well. Expect is not a solution because I don't want to talk to the child, I want to get status information in a machine readable way back from it (+displaying the output in a terminal).

The fact that the patch is implemented as a environment variable was to keep it simple (and yes, that's a bit of a hack :)

What I really want is a way to run a command in vte (for fork a pty) and tell libvte to not close the file descriptors (or some file descriptors). I can provide a patch that has a property instead of the environment if you like it better this way. I'm happy to provide more information if needed.

Comment 4 Behdad Esfahbod 2006-04-12 06:50:10 UTC
Yes, a property looks like a sane approach to me.
Comment 5 Behdad Esfahbod 2006-04-20 13:51:40 UTC
I committed a patch that implements vte_terminal_set_pty.  That doesn't quite do what you want, but is enough if you are willing to implement your own openpty...
Comment 6 Chris Wilson 2007-02-01 01:21:14 UTC
I guess the easiest way to implement this (wrt g_spawn) is to pass down GSpawnFlags to _vte_pty_fork_on_pty_fd() from _vte_pty_open(). Again not quite what you want since it's all or nothing...
Comment 7 Chris Wilson 2007-02-01 01:28:11 UTC
... except I forgot that one cannot simply do that with an exposed API.
Comment 8 Behdad Esfahbod 2007-02-01 19:43:11 UTC
We can always add API.  What about allow passing a callback to call in child_setup?  Probably still need to allow GSpawnFlags to keep them open.  Given the two, you have the option to close only the ones you want.
Comment 9 Michael Vogt 2007-02-02 19:55:33 UTC
Being able to pass a flag (or setting a property) that can keep all FDs open should be fine. I can start looking into it next week and update my patch.
Comment 10 Christian Persch 2008-07-15 11:35:46 UTC
Did you ever get around to updating the patch? :)
Comment 11 Daniel Burrows 2008-10-19 21:09:54 UTC
Created attachment 120883 [details] [review]
A patch to add a new variant of forkpty() that doesn't close the currently open file descriptors.
Comment 12 Daniel Burrows 2008-10-19 21:10:51 UTC
I just ran into this while I was reading synaptic's source code.  I put together a quick patch that just adds a forkpty() variant with support for not closing file descriptors.  It's not as general as passing in a fork()-alike callback, but it was really easy to write and will solve this problem.  I also documented the fd-closing behavior of the plain forkpty, since I found it rather startling when I ran into it.

Note: I haven't tested this code at all, except to verify that I can still run synaptic and install packages with it.  In particular, I haven't tested that file descriptors really stay open when you use the new variant.
Comment 13 Christian Persch 2008-10-19 22:33:10 UTC
Created attachment 120892 [details] [review]
[PATCH] Bug 320128 – [patch] please provide a way to keep fds open with "vte_terminal_fork_pty"

 src/pty.c |   73 +++++++++++++++++++++-------
 src/pty.h |    4 +-
 src/vte.c |  159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 src/vte.h |   20 ++++++++
 4 files changed, 221 insertions(+), 35 deletions(-)
Comment 14 Christian Persch 2008-10-19 22:34:11 UTC
My patch implements passing the GSpawnFlags and an optional extra GSpawnChildSetupFunc, as suggested in comment 8.
Comment 15 Behdad Esfahbod 2008-10-20 04:58:14 UTC
Looks good.  Nice work!
Comment 16 Christian Persch 2008-10-20 18:21:02 UTC
So the naming vte_terminal_fork*_full is ok? Should we go for the whole thing and also add outparams for stdin/out/err that g_spawn_async_with_pipes provide, or will there be no use for them?
Comment 17 Behdad Esfahbod 2008-10-20 18:27:03 UTC
_full is good.  If we're doing full, make it as full as you can possibly imagine ;).
Comment 18 Christian Persch 2008-10-23 10:26:38 UTC
Well, I was asking because I don't know how useful the stdin/out/err fds will be.

It's easy to implement in vte_terminal_fork_command_full() since it uses g_spawn_async_with_pipes(). But vte_terminal_forkpty*() just uses fork(), so I'd have to copy a lot of code from glib/gspawn.c here. 

I could either just add the outparams but declare them currently-unused/to-be-implemented-later, or just leave them out and if someone does come up with a use for them later, we can add vte_terminal_forkpty_with_pipes()...
Comment 19 Christian Persch 2008-10-23 10:40:33 UTC
Another thing: in pty.c there's this comment:

/* TODO: clean up the spawning
 * - replace current env rather than adding!
 * - allow user control over flags (eg DO_NOT_CLOSE)
 * - additional user callback for child setup

The patch here addresses points 2 and 3; how about 1? Should we have a "gboolean replace_env" in the _full variants?
Comment 20 Daniel Burrows 2008-10-23 14:23:11 UTC
For our use case, we have no need to access the stdin/stdout/stderr file descriptors.  We just need an extra descriptor for out-of-band communications with the subprocess.
Comment 21 Behdad Esfahbod 2008-10-23 16:29:12 UTC
(In reply to comment #19)
> Another thing: in pty.c there's this comment:
> 
> /* TODO: clean up the spawning
>  * - replace current env rather than adding!
>  * - allow user control over flags (eg DO_NOT_CLOSE)
>  * - additional user callback for child setup
> 
> The patch here addresses points 2 and 3; how about 1? Should we have a
> "gboolean replace_env" in the _full variants?

Or add G_SPAWN_APPEND_ENV to gspawn?
Comment 22 Daniel Burrows 2009-02-07 21:06:49 UTC
Are the patches in this bug going to be applied?
Comment 23 Christian Persch 2010-03-17 19:23:28 UTC
The patches are obsolete. The vte_terminal_forkpty API is deprecated. You can now just do pty = vte_pty_new (0); fork(), call vte_pty_child_setup in the child and in the parent use vte_terminal_set_pty_object(terminal, pty) (untested; if it doesn't work please file a new bug). AFAICT nothing in this sequence should mess with your open FDs anymore, so I think this bug is FIXED or OBSOLETE.