[Bug 1330] RFE: 'ControlPersist' support -- automatically fork and leave ControlMaster behind as a dæmon

[bugzilla-daemon at bugzilla.mindrot.org]

http://bugzilla.mindrot.org/show_bug.cgi?id=1330





--- Comment #6 from Damien Miller <djm at mindrot.org>  2007-07-23 14:29:27 ---
(From update of attachment 1331)
Some comments on the diff:

>diff -ru openssh-4.6p1-orig/clientloop.c openssh-4.6p1-idletimeout/clientloop.c
>--- openssh-4.6p1-orig/clientloop.c	2007-07-10 12:48:22.000000000 +0200
>+++ openssh-4.6p1-idletimeout/clientloop.c	2007-07-23 03:51:13.000000000 +0200
>-	if (options.server_alive_interval == 0 || !compat20)
>-		tvp = NULL;
>-	else {
>-		tv.tv_sec = options.server_alive_interval;
>-		tv.tv_usec = 0;
>-		tvp = &tv;
>-	}
>-	ret = select((*maxfdp)+1, *readsetp, *writesetp, NULL, tvp);
>+	ret = packet_select((*maxfdp)+1, *readsetp, *writesetp, NULL,
>+			compat20 ? options.server_alive_interval : 0);

I think it would be more clear to create a calculate_select_timeout()
function (or somesuch) that returns a timeval* rather than making a new
API around select()

>+	} else if (ret == 0) {
>+		if (packet_idle_timedout()) {
>+			char buf[100];
>+			snprintf(buf, sizeof buf, "Idle timeout.\r\n");
>+			buffer_append(&stderr_buffer, buf, strlen(buf));
>+			quit_pending = 1;

Why not use logit() or verbose() here?

> static void
>@@ -1343,8 +1344,13 @@
> client_channel_closed(int id, void *arg)
> {
> 	channel_cancel_cleanup(id);
>-	session_closed = 1;
> 	leave_raw_mode();
>+	if (options.control_persist && options.control_path != NULL && control_fd != -1) {
>+		packet_set_idle_timeout(options.control_timeout);
>+		daemon(0,0);
>+		return;

As mentioned in comment #2, I'd prefer to see the ControlPersist master
background itself when the connection is established (but after
authentication - e.g. as the -f option does it) and have the
foregrounded ssh process become a client of it.


>diff -ru openssh-4.6p1-orig/readconf.c openssh-4.6p1-idletimeout/readconf.c
>--- openssh-4.6p1-orig/readconf.c	2007-07-10 12:48:22.000000000 +0200
>+++ openssh-4.6p1-idletimeout/readconf.c	2007-07-20 21:35:15.000000000 +0200
>@@ -221,11 +221,14 @@
> 	{ "sendenv", oSendEnv },
> 	{ "controlpath", oControlPath },
> 	{ "controlmaster", oControlMaster },
>+	{ "controlpersist", oControlPersist },
>+	{ "controltimeout", oControlTimeout },

These would be better off as a single ControlPersist option that
accepted "yes"/"true", "no"/"false" or a timeout parsed using
misc.c:convtime()

E.g. "ControlPersist 5m"

>+	{ "idletimeout", oIdleTimeout },

IdleTimeout is a separate issue, it can stay in here for now but it
will need to be removed to a separate patch before the ControlPersist
stuff goes in.


-- 
Configure bugmail: http://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.