GNOME Bugzilla – Bug 320128
[patch] please provide a way to keep fds open with "vte_terminal_fork_pty"
Last modified: 2011-05-04 11:40:39 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 :)
Created attachment 54017 [details] [review] patch that implements a VTE_KEEP_FD_OPEN enviroment
Ehr.. so this is a vte specific hack and other terminals will not have it? What about using expect or something?
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.
Yes, a property looks like a sane approach to me.
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...
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...
... except I forgot that one cannot simply do that with an exposed API.
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.
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.
Did you ever get around to updating the patch? :)
Created attachment 120883 [details] [review] A patch to add a new variant of forkpty() that doesn't close the currently open file descriptors.
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.
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(-)
My patch implements passing the GSpawnFlags and an optional extra GSpawnChildSetupFunc, as suggested in comment 8.
Looks good. Nice work!
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?
_full is good. If we're doing full, make it as full as you can possibly imagine ;).
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()...
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?
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.
(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?
Are the patches in this bug going to be applied?
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.