Re: [patch] [gdbserver] Fix multi-GB error log files [doc review]

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Sat, 23 Apr 2011 08:15:38 +0200, Eli Zaretskii wrote:
>   GDBserver now keeps its listening port open, unless you start
>   GDBserver with the --once option.  Keeping the listening port open
>   allows subsequent reconnections from another GDB session. 

BTW personally I read it as if GDBserver did not support reconnections before
and that reconnections are a new feature (which they are not).


> > +                                Option @option{--multi} is not directly related
> > +to @kbd{target extended-remote} (therefore @kbd{target remote} is also
> > +compatible) and vice versa.
> 
> It's not clear to me how this sentence is related to the surrounding
> text.  What potential problem did you try to prevent by that?

Yes, I was considering to post it as a separate patch but (a) it would be
a hassle for one sentence and (b) it is all related together anyway.

--multi, remote/extended-remote and --once all together affect when gdbserver
will / will not terminate.  Normally AFAIK one uses either:
	--multi with extended-remote
or
	(default) with remote
so I was curious if one can mix the two modes.  And to my surprise gdbserver
behavior in which cases it terminates depends on how GDB connects to it
(remote/extended-remote) instead of the gdbserver arguments (--multi).
This is why this question seemed to me logical in the context of explaining
all the GDB termination rules which needed to be explained together with the
port allocation rules.


> > +terminated in @kbd{target remote} mode.  On the other hand for @kbd{target
>                                                         ^
> Missing comma there.

You wrote it as "On the other, hand for " but I have used "On the other hand,
for ", either a typo or a font layout problem on your side.


> > +                                                The purpose of option
> > +@option{--once} is to enable reusing the same port number for connections to
> > +multiple @code{gdbserver}s running at the same host.
> 
> "The purpose of the @option{--once} option is ..."
> 
> However, is the above the only purpose of this option?  If not, saying
> "the purpose" is inaccurate.

Not the only one, it is also useful if any unexpected situation happens in the
testsuite gdbserver will no longer be needlessly running.

> How about the following rewording:

Used.

> 	* gdb.texinfo (Server): Improve indexing.  Index all optional
> 	switches to gdbserver.

Added @cindex for --once.

Used all your other suggestions, I am not aware much of the commas.

I will check it in if no futher comments appear.


Thanks,
Jan


gdb/gdbserver/
2011-04-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* NEWS: Document the new gdbserver --once option.
	* remote-utils.c (handle_accept_event): Close LISTEN_DESC only if
	RUN_ONCE.  Comment for the LISTEN_DESC delete_file_handler call.
	(remote_prepare): New function with most of the TCP code from ...
	(remote_open): ... here.  Detect PORT here unconditionally.  Move also
	setting transport_is_reliable.
	* server.c (run_once): New variable.
	(gdbserver_usage): Document it.
	(main): Set run_once for `--once'.  Call remote_prepare.  Exit after
	the first run if RUN_ONCE.
	* server.h (run_once, remote_prepare): New declarations.

gdb/doc/
2011-04-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Eli Zaretskii  <eliz@gnu.org>

	* gdb.texinfo (Starting and Stopping Trace Experiments): New anchor
	for disconnected tracing.
	(Multi-Process Mode for @code{gdbserver}): Mention --multi and
	extended-remote relationship.  Mention --once.
	(TCP port allocation lifecycle of @code{gdbserver}): New.

gdb/testsuite/
2011-04-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/solib-disc.exp: Set gdbserver_reconnect_p.
	* lib/gdb.exp (gdb_init): Clear gdbserver_reconnect_p.
	* lib/gdbserver-support.exp (gdbserver_start): Add `--once' if
	!gdbserver_reconnect_p..
	(gdbserver_reconnect): Call error if !gdbserver_reconnect_p..

--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,10 @@
 
 *** Changes since GDB 7.3
 
+* GDBserver now keeps its listening port open, unless you start GDBserver with
+  the --once option.  Keeping the listening port open allows subsequent
+  reconnections from another GDB session.
+
 *** Changes in GDB 7.3
 
 * GDB has a new command: "thread find [REGEXP]".
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10435,6 +10435,7 @@ Enter actions for tracepoint #1, one per line.
 (@value{GDBP}) @b{tstop}
 @end smallexample
 
+@anchor{disconnected tracing}
 @cindex disconnected tracing
 You can choose to continue running the trace experiment even if
 @value{GDBN} disconnects from the target, voluntarily or
@@ -16230,11 +16231,43 @@ redirection (@pxref{Arguments}).
 To start @code{gdbserver} without supplying an initial command to run
 or process ID to attach, use the @option{--multi} command line option.
 Then you can connect using @kbd{target extended-remote} and start
-the program you want to debug.
-
-@code{gdbserver} does not automatically exit in multi-process mode.
-You can terminate it by using @code{monitor exit}
-(@pxref{Monitor Commands for gdbserver}).
+the program you want to debug.  The @option{--multi} option is not directly
+related to @kbd{target extended-remote} (therefore @kbd{target remote} is also
+compatible) and vice versa.
+
+In multi-process mode @code{gdbserver} does not automatically exit unless you
+use the option @option{--once}.  You can terminate it by using
+@code{monitor exit} (@pxref{Monitor Commands for gdbserver}).
+
+@subsubsection TCP port allocation lifecycle of @code{gdbserver}
+
+This section applies only when @code{gdbserver} is run to listen on a TCP port.
+
+@code{gdbserver} normally terminates after all of its debugged processes have
+terminated in @kbd{target remote} mode.  On the other hand, for @kbd{target
+extended-remote}, @code{gdbserver} stays running even with no processes left.
+@value{GDBN} normally terminates the spawned debugged process on its exit,
+which normally also terminates @code{gdbserver} in the @kbd{target remote}
+mode.  Therefore, when the connection drops unexpectedly, and @value{GDBN}
+cannot ask @code{gdbserver} to kill its debugged processes, @code{gdbserver}
+stays running even in the @kbd{target remote} mode.
+
+When @code{gdbserver} stays running, @value{GDBN} can connect to it again later.
+Such reconnecting is useful for features like @ref{disconnected tracing}.  For
+completeness, at most one @value{GDBN} can be connected at a time.
+
+@cindex @option{--once}, @code{gdbserver} option
+By default, @code{gdbserver} keeps the listening TCP port open, so that
+additional connections are possible.  However, if you start @code{gdbserver}
+with the @option{--once} option, it will stop listening for any further
+connection attempts after connecting to the first @value{GDBN} session.  This
+means no further connections to @code{gdbserver} will be possible after the
+first one.  It also means @code{gdbserver} will terminate after the first
+connection with remote @value{GDBN} has closed, even for unexpectedly closed
+connections and even in the @kbd{target extended-remote} mode.  The
+@option{--once} option allows reusing the same port number for connecting to
+multiple instances of @code{gdbserver} running on the same host, since each
+instance closes its port after the first connection.
 
 @subsubsection Other Command-Line Arguments for @code{gdbserver}
 
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -170,14 +170,21 @@ handle_accept_event (int err, gdb_client_data client_data)
 	      (char *) &tmp, sizeof (tmp));
 
 #ifndef USE_WIN32API
-  close (listen_desc);		/* No longer need this */
-
   signal (SIGPIPE, SIG_IGN);	/* If we don't do this, then gdbserver simply
 				   exits when the remote side dies.  */
+#endif
+
+  if (run_once)
+    {
+#ifndef USE_WIN32API
+      close (listen_desc);		/* No longer need this */
 #else
-  closesocket (listen_desc);	/* No longer need this */
+      closesocket (listen_desc);	/* No longer need this */
 #endif
+    }
 
+  /* Even if !RUN_ONCE no longer notice new connections.  Still keep the
+     descriptor open for add_file_handler to wait for a new connection.  */
   delete_file_handler (listen_desc);
 
   /* Convert IP address to string.  */
@@ -200,6 +207,62 @@ handle_accept_event (int err, gdb_client_data client_data)
   return 0;
 }
 
+/* Prepare for a later connection to a remote debugger.
+   NAME is the filename used for communication.  */
+
+void
+remote_prepare (char *name)
+{
+  char *port_str;
+#ifdef USE_WIN32API
+  static int winsock_initialized;
+#endif
+  int port;
+  struct sockaddr_in sockaddr;
+  socklen_t tmp;
+  char *port_end;
+
+  port_str = strchr (name, ':');
+  if (port_str == NULL)
+    {
+      transport_is_reliable = 0;
+      return;
+    }
+
+  port = strtoul (port_str + 1, &port_end, 10);
+  if (port_str[1] == '\0' || *port_end != '\0')
+    fatal ("Bad port argument: %s", name);
+
+#ifdef USE_WIN32API
+  if (!winsock_initialized)
+    {
+      WSADATA wsad;
+
+      WSAStartup (MAKEWORD (1, 0), &wsad);
+      winsock_initialized = 1;
+    }
+#endif
+
+  listen_desc = socket (PF_INET, SOCK_STREAM, IPPROTO_TCP);
+  if (listen_desc == -1)
+    perror_with_name ("Can't open socket");
+
+  /* Allow rapid reuse of this port. */
+  tmp = 1;
+  setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
+	      sizeof (tmp));
+
+  sockaddr.sin_family = PF_INET;
+  sockaddr.sin_port = htons (port);
+  sockaddr.sin_addr.s_addr = INADDR_ANY;
+
+  if (bind (listen_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
+      || listen (listen_desc, 1))
+    perror_with_name ("Can't bind address");
+
+  transport_is_reliable = 1;
+}
+
 /* Open a connection to a remote debugger.
    NAME is the filename used for communication.  */
 
@@ -274,8 +337,6 @@ remote_open (char *name)
 
       fprintf (stderr, "Remote debugging using %s\n", name);
 
-      transport_is_reliable = 0;
-
       enable_async_notification (remote_desc);
 
       /* Register the event loop handler.  */
@@ -284,64 +345,22 @@ remote_open (char *name)
     }
   else
     {
-#ifdef USE_WIN32API
-      static int winsock_initialized;
-#endif
       int port;
+      socklen_t len;
       struct sockaddr_in sockaddr;
-      socklen_t tmp;
-      char *port_end;
 
-      port = strtoul (port_str + 1, &port_end, 10);
-      if (port_str[1] == '\0' || *port_end != '\0')
-	fatal ("Bad port argument: %s", name);
-
-#ifdef USE_WIN32API
-      if (!winsock_initialized)
-	{
-	  WSADATA wsad;
-
-	  WSAStartup (MAKEWORD (1, 0), &wsad);
-	  winsock_initialized = 1;
-	}
-#endif
-
-      listen_desc = socket (PF_INET, SOCK_STREAM, IPPROTO_TCP);
-      if (listen_desc == -1)
-	perror_with_name ("Can't open socket");
-
-      /* Allow rapid reuse of this port. */
-      tmp = 1;
-      setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
-		  sizeof (tmp));
-
-      sockaddr.sin_family = PF_INET;
-      sockaddr.sin_port = htons (port);
-      sockaddr.sin_addr.s_addr = INADDR_ANY;
-
-      if (bind (listen_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
-	  || listen (listen_desc, 1))
-	perror_with_name ("Can't bind address");
-
-      /* If port is zero, a random port will be selected, and the
-	 fprintf below needs to know what port was selected.  */
-      if (port == 0)
-	{
-	  socklen_t len = sizeof (sockaddr);
-	  if (getsockname (listen_desc,
-			   (struct sockaddr *) &sockaddr, &len) < 0
-	      || len < sizeof (sockaddr))
-	    perror_with_name ("Can't determine port");
-	  port = ntohs (sockaddr.sin_port);
-	}
+      len = sizeof (sockaddr);
+      if (getsockname (listen_desc,
+		       (struct sockaddr *) &sockaddr, &len) < 0
+	  || len < sizeof (sockaddr))
+	perror_with_name ("Can't determine port");
+      port = ntohs (sockaddr.sin_port);
 
       fprintf (stderr, "Listening on port %d\n", port);
       fflush (stderr);
 
       /* Register the event loop handler.  */
       add_file_handler (listen_desc, handle_accept_event, NULL);
-
-      transport_is_reliable = 1;
     }
 }
 
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -40,6 +40,9 @@ static int extended_protocol;
 static int response_needed;
 static int exit_requested;
 
+/* --once: Exit after the first connection has closed.  */
+int run_once;
+
 int multi_process;
 int non_stop;
 
@@ -2312,7 +2315,9 @@ gdbserver_usage (FILE *stream)
 	   "  --debug               Enable general debugging output.\n"
 	   "  --remote-debug        Enable remote protocol debugging output.\n"
 	   "  --version             Display version information and exit.\n"
-	   "  --wrapper WRAPPER --  Run WRAPPER to start new programs.\n");
+	   "  --wrapper WRAPPER --  Run WRAPPER to start new programs.\n"
+	   "  --once                Exit after the first connection has "
+								  "closed.\n");
   if (REPORT_BUGS_TO[0] && stream == stdout)
     fprintf (stream, "Report bugs to \"%s\".\n", REPORT_BUGS_TO);
 }
@@ -2536,6 +2541,8 @@ main (int argc, char *argv[])
 		}
 	    }
 	}
+      else if (strcmp (*next_arg, "--once") == 0)
+	run_once = 1;
       else
 	{
 	  fprintf (stderr, "Unknown argument: %s\n", *next_arg);
@@ -2648,6 +2655,8 @@ main (int argc, char *argv[])
       exit (1);
     }
 
+  remote_prepare (port);
+
   while (1)
     {
       noack_mode = 0;
@@ -2676,7 +2685,7 @@ main (int argc, char *argv[])
 	 getpkt to fail; close the connection and reopen it at the
 	 top of the loop.  */
 
-      if (exit_requested)
+      if (exit_requested || run_once)
 	{
 	  detach_or_kill_for_exit ();
 	  exit (0);
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -355,6 +355,7 @@ extern int disable_packet_Tthread;
 extern int disable_packet_qC;
 extern int disable_packet_qfThreadInfo;
 
+extern int run_once;
 extern int multi_process;
 extern int non_stop;
 
@@ -406,6 +407,7 @@ int putpkt (char *buf);
 int putpkt_binary (char *buf, int len);
 int putpkt_notif (char *buf);
 int getpkt (char *buf);
+void remote_prepare (char *name);
 void remote_open (char *name);
 void remote_close (void);
 void write_ok (char *buf);
--- a/gdb/testsuite/gdb.base/solib-disc.exp
+++ b/gdb/testsuite/gdb.base/solib-disc.exp
@@ -19,6 +19,7 @@ if {[skip_shlib_tests]} {
     return 0
 }
 
+set gdbserver_reconnect_p 1
 if { [info proc gdb_reconnect] == "" } {
     return 0
 }
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2809,6 +2809,11 @@ proc gdb_init { args } {
     # especially having color output turned on can cause tests to fail.
     setenv GREP_OPTIONS ""
 
+    # Clear $gdbserver_reconnect_p.
+    global gdbserver_reconnect_p
+    set gdbserver_reconnect_p 1
+    unset gdbserver_reconnect_p
+
     return [eval default_gdb_init $args];
 }
 
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -218,10 +218,21 @@ proc gdbserver_start { options arguments } {
 
 	# Fire off the debug agent.
 	set gdbserver_command "$gdbserver"
+
+	# If gdbserver_reconnect will be called $gdbserver_reconnect_p must be
+	# set to true already during gdbserver_start.
+	global gdbserver_reconnect_p
+	if {![info exists gdbserver_reconnect_p] || !$gdbserver_reconnect_p} {
+	    # GDB client could accidentally connect to a stale server.
+	    append gdbserver_command " --once"
+	}
+
 	if { $options != "" } {
 	    append gdbserver_command " $options"
 	}
+
 	append gdbserver_command " :$portnum"
+
 	if { $arguments != "" } {
 	    append gdbserver_command " $arguments"
 	}
@@ -315,6 +326,12 @@ proc gdbserver_reconnect { } {
     global gdbserver_protocol
     global gdbserver_gdbport
 
+    global gdbserver_reconnect_p;
+    if {![info exists gdbserver_reconnect_p] || !$gdbserver_reconnect_p} {
+	error "gdbserver_reconnect_p is not set before gdbserver_reconnect"
+	return 0
+    }
+
     return [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
 }