[patch] set logging {redirect|overwrite} warning

[patch] set logging {redirect|overwrite} warning

[Jan Kratochvil]

Hi,

downstream Bug:
https://bugzilla.redhat.com/show_bug.cgi?id=593521

After
(gdb) set logging on

any settings like
(gdb) set logging redirect on
or
(gdb) set logging overwrite on

get silently set but ... they do not work.  They get active only after next
(gdb) set logging off
(gdb) set logging on


I understand one a GDB patch could make them active immediately etc.  The
patch provided below is safe for regressions, it fixes the problem, it was
easy to develop, it does not complicate the code for future fixes/extensions.

(gdb) set logging on
Copying output to gdb.txt.
(gdb) set logging redirect 
Already logging to gdb.txt.  You should turn the logging off and on to make the new setting effective.

No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.


Thanks,
Jan


gdb/
2010-08-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* cli/cli-logging.c (set_logging_overwrite, set_logging_redirect): New
	functions.
	(_initialize_cli_logging): Install them.

--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -46,6 +46,17 @@ show_logging_filename (struct ui_file *file, int from_tty,
 }
 
 int logging_overwrite;
+
+static void
+set_logging_overwrite (char *args, int from_tty, struct cmd_list_element *c)
+{
+  if (saved_filename)
+    fprintf_unfiltered (gdb_stdout, _("Already logging to %s.  You should "
+				      "turn the logging off and on to make "
+				      "the new setting effective.\n"),
+			saved_filename);
+}
+
 static void
 show_logging_overwrite (struct ui_file *file, int from_tty,
 			struct cmd_list_element *c, const char *value)
@@ -56,6 +67,17 @@ Whether logging overwrites or appends to the log file is %s.\n"),
 }
 
 int logging_redirect;
+
+static void
+set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
+{
+  if (saved_filename)
+    fprintf_unfiltered (gdb_stdout, _("Already logging to %s.  You should "
+				      "turn the logging off and on to make "
+				      "the new setting effective.\n"),
+			saved_filename);
+}
+
 static void
 show_logging_redirect (struct ui_file *file, int from_tty,
 		       struct cmd_list_element *c, const char *value)
@@ -211,7 +233,7 @@ _initialize_cli_logging (void)
 Set whether logging overwrites or appends to the log file."), _("\
 Show whether logging overwrites or appends to the log file."), _("\
 If set, logging overrides the log file."),
-			   NULL,
+			   set_logging_overwrite,
 			   show_logging_overwrite,
 			   &set_logging_cmdlist, &show_logging_cmdlist);
   add_setshow_boolean_cmd ("redirect", class_support, &logging_redirect, _("\
@@ -219,7 +241,7 @@ Set the logging output mode."), _("\
 Show the logging output mode."), _("\
 If redirect is off, output will go to both the screen and the log file.\n\
 If redirect is on, output will go only to the log file."),
-			   NULL,
+			   set_logging_redirect,
 			   show_logging_redirect,
 			   &set_logging_cmdlist, &show_logging_cmdlist);
   add_setshow_filename_cmd ("file", class_support, &logging_filename, _("\

Re: [patch] set logging {redirect|overwrite} warning

[Pedro Alves]

On Friday 06 August 2010 19:29:19, Jan Kratochvil wrote:

> (gdb) set logging on
> Copying output to gdb.txt.
> (gdb) set logging redirect 
> Already logging to gdb.txt.  You should turn the logging off and on to make the new setting effective.

This is fine with me.  "set logging redirect" could take affect on
the fly; "set logging overwrite", not so clear.

> +static void
> +set_logging_overwrite (char *args, int from_tty, struct cmd_list_element *c)
> +{
> +  if (saved_filename)
> +    fprintf_unfiltered (gdb_stdout, _("Already logging to %s.  You should "
> +				      "turn the logging off and on to make "
> +				      "the new setting effective.\n"),
> +			saved_filename);

Any reason to not use "warning" instead of "fprintf_unfiltered"?
(if you switch, remember to drop \n).

I'd suggest s/Already/Currently/ and s/You should//.

Since there's more than one place with the same string, it'd be nice
to abstract that out to a "warn_reenable_logging_to_take_effect"
function, or some such.

-- 
Pedro Alves

Re: [patch] set logging {redirect|overwrite} warning

[Jan Kratochvil]

On Thu, 12 Aug 2010 18:07:46 +0200, Pedro Alves wrote:
> "set logging redirect" could take affect on the fly;

OK... implemented.

> Any reason to not use "warning" instead of "fprintf_unfiltered"?
> (if you switch, remember to drop \n).

Done.

> I'd suggest s/Already/Currently/ and s/You should//.

Done.

> Since there's more than one place with the same string, it'd be nice
> to abstract that out to a "warn_reenable_logging_to_take_effect"
> function, or some such.

As there remainde only one warning now it is no longer applicable.
(This remained warning is not much important/useful anyway.)


No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.

OK to check-in?


Thanks,
Jan


2010-08-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* cli-logging.c: Include gdb_assert.h.
	(set_logging_overwrite): New function.
	(logging_redirect): New comment.
	(logging_no_redirect_file, set_logging_redirect)
	(pop_output_files) <logging_no_redirect_file>: New.
	(handle_redirections) <!logging_redirect>: New variable
	no_redirect_file.  Remove file autoclose for tee_file_new.  No longer
	discard cleanup for the close of former OUTPUT.  Set
	LOGGING_NO_REDIRECT_FILE.
	(handle_redirections) <logging_redirect>: gdb_assert
	LOGGING_NO_REDIRECT_FILE.
	(show_logging_command) <logging_redirect handling>: Adjust messages
	for SAVED_FILENAME not NULL.
	(_initialize_cli_logging): Install set_logging_overwrite and
	set_logging_redirect.

--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -21,6 +21,7 @@
 #include "defs.h"
 #include "gdbcmd.h"
 #include "ui-out.h"
+#include "gdb_assert.h"
 
 #include "gdb_string.h"
 
@@ -46,6 +47,15 @@ show_logging_filename (struct ui_file *file, int from_tty,
 }
 
 static int logging_overwrite;
+
+static void
+set_logging_overwrite (char *args, int from_tty, struct cmd_list_element *c)
+{
+  if (saved_filename)
+    warning (_("Currently logging to %s.  Turn the logging off and on to "
+	       "make the new setting effective."), saved_filename);
+}
+
 static void
 show_logging_overwrite (struct ui_file *file, int from_tty,
 			struct cmd_list_element *c, const char *value)
@@ -55,7 +65,67 @@ Whether logging overwrites or appends to the log file is %s.\n"),
 		    value);
 }
 
+/* Value as configured by the user.  */
 static int logging_redirect;
+
+/* The on-disk file in use if logging is currently active together with
+   redirection turned off (and therefore using tee_file_new).  For active
+   logging with redirection the on-disk file is directly in GDB_STDOUT and
+   this variable is NULL.  */
+static struct ui_file *logging_no_redirect_file;
+
+static void
+set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
+{
+  struct cleanup *cleanups = NULL;
+  struct ui_file *output, *new_logging_no_redirect_file;
+
+  if (saved_filename == NULL
+      || (logging_redirect != 0 && logging_no_redirect_file == NULL)
+      || (logging_redirect == 0 && logging_no_redirect_file != NULL))
+    return;
+
+  if (logging_redirect != 0)
+    {
+      gdb_assert (logging_no_redirect_file != NULL);
+
+      /* ui_out_redirect still has not been called for next gdb_stdout.  */
+      cleanups = make_cleanup_ui_file_delete (gdb_stdout);
+
+      output = logging_no_redirect_file;
+      new_logging_no_redirect_file = NULL;
+
+      if (from_tty)
+	fprintf_unfiltered (saved_output.out, "Redirecting output to %s.\n",
+			    logging_filename);
+    }
+  else
+    {
+      gdb_assert (logging_no_redirect_file == NULL);
+      output = tee_file_new (saved_output.out, 0, gdb_stdout, 0);
+      if (output == NULL)
+	perror_with_name (_("set logging"));
+      new_logging_no_redirect_file = gdb_stdout;
+
+      if (from_tty)
+	fprintf_unfiltered (saved_output.out, "Copying output to %s.\n",
+			    logging_filename);
+    }
+
+  gdb_stdout = output;
+  gdb_stderr = output;
+  gdb_stdlog = output;
+  gdb_stdtarg = output;
+  logging_no_redirect_file = new_logging_no_redirect_file;
+
+  /* It should not happen, the redirection has been already setup.  */
+  if (ui_out_redirect (uiout, output) < 0)
+    warning (_("Current output protocol does not support redirection"));
+
+  if (logging_redirect != 0)
+    do_cleanups (cleanups);
+}
+
 static void
 show_logging_redirect (struct ui_file *file, int from_tty,
 		       struct cmd_list_element *c, const char *value)
@@ -70,6 +140,11 @@ pop_output_files (void)
   /* Only delete one of the files -- they are all set to the same
      value.  */
   ui_file_delete (gdb_stdout);
+  if (logging_no_redirect_file)
+    {
+      ui_file_delete (logging_no_redirect_file);
+      logging_no_redirect_file = NULL;
+    }
   gdb_stdout = saved_output.out;
   gdb_stderr = saved_output.err;
   gdb_stdlog = saved_output.log;
@@ -104,18 +179,25 @@ handle_redirections (int from_tty)
   /* Redirects everything to gdb_stdout while this is running.  */
   if (!logging_redirect)
     {
-      output = tee_file_new (gdb_stdout, 0, output, 1);
+      struct ui_file *no_redirect_file = output;
+
+      output = tee_file_new (gdb_stdout, 0, no_redirect_file, 0);
       if (output == NULL)
 	perror_with_name (_("set logging"));
-      discard_cleanups (cleanups);
-      cleanups = make_cleanup_ui_file_delete (output);
+      make_cleanup_ui_file_delete (output);
       if (from_tty)
 	fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
 			    logging_filename);
+      logging_no_redirect_file = no_redirect_file;
+    }
+  else
+    {
+      gdb_assert (logging_no_redirect_file == NULL);
+
+      if (from_tty)
+	fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
+			    logging_filename);
     }
-  else if (from_tty)
-    fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
-			logging_filename);
 
   discard_cleanups (cleanups);
 
@@ -187,10 +269,20 @@ show_logging_command (char *args, int from_tty)
   else
     printf_unfiltered (_("Logs will be appended to the log file.\n"));
 
-  if (logging_redirect)
-    printf_unfiltered (_("Output will be sent only to the log file.\n"));
+  if (saved_filename)
+    {
+      if (logging_redirect)
+	printf_unfiltered (_("Output is being sent only to the log file.\n"));
+      else
+	printf_unfiltered (_("Output is being logged and displayed.\n"));
+    }
   else
-    printf_unfiltered (_("Output will be logged and displayed.\n"));
+    {
+      if (logging_redirect)
+	printf_unfiltered (_("Output will be sent only to the log file.\n"));
+      else
+	printf_unfiltered (_("Output will be logged and displayed.\n"));
+    }
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
@@ -211,7 +303,7 @@ _initialize_cli_logging (void)
 Set whether logging overwrites or appends to the log file."), _("\
 Show whether logging overwrites or appends to the log file."), _("\
 If set, logging overrides the log file."),
-			   NULL,
+			   set_logging_overwrite,
 			   show_logging_overwrite,
 			   &set_logging_cmdlist, &show_logging_cmdlist);
   add_setshow_boolean_cmd ("redirect", class_support, &logging_redirect, _("\
@@ -219,7 +311,7 @@ Set the logging output mode."), _("\
 Show the logging output mode."), _("\
 If redirect is off, output will go to both the screen and the log file.\n\
 If redirect is on, output will go only to the log file."),
-			   NULL,
+			   set_logging_redirect,
 			   show_logging_redirect,
 			   &set_logging_cmdlist, &show_logging_cmdlist);
   add_setshow_filename_cmd ("file", class_support, &logging_filename, _("\

ping: [patch] set logging {redirect|overwrite} warning

[Jan Kratochvil]

There are some more dependencies around these redirections to fix so this one
could go in first:

On Fri, 13 Aug 2010 23:55:42 +0200, Jan Kratochvil wrote:
On Thu, 12 Aug 2010 18:07:46 +0200, Pedro Alves wrote:
> "set logging redirect" could take affect on the fly;

OK... implemented.

> Any reason to not use "warning" instead of "fprintf_unfiltered"?
> (if you switch, remember to drop \n).

Done.

> I'd suggest s/Already/Currently/ and s/You should//.

Done.

> Since there's more than one place with the same string, it'd be nice
> to abstract that out to a "warn_reenable_logging_to_take_effect"
> function, or some such.

As there remainde only one warning now it is no longer applicable.
(This remained warning is not much important/useful anyway.)


No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.

OK to check-in?


Thanks,
Jan


2010-08-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* cli-logging.c: Include gdb_assert.h.
	(set_logging_overwrite): New function.
	(logging_redirect): New comment.
	(logging_no_redirect_file, set_logging_redirect)
	(pop_output_files) <logging_no_redirect_file>: New.
	(handle_redirections) <!logging_redirect>: New variable
	no_redirect_file.  Remove file autoclose for tee_file_new.  No longer
	discard cleanup for the close of former OUTPUT.  Set
	LOGGING_NO_REDIRECT_FILE.
	(handle_redirections) <logging_redirect>: gdb_assert
	LOGGING_NO_REDIRECT_FILE.
	(show_logging_command) <logging_redirect handling>: Adjust messages
	for SAVED_FILENAME not NULL.
	(_initialize_cli_logging): Install set_logging_overwrite and
	set_logging_redirect.

--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -21,6 +21,7 @@
 #include "defs.h"
 #include "gdbcmd.h"
 #include "ui-out.h"
+#include "gdb_assert.h"
 
 #include "gdb_string.h"
 
@@ -46,6 +47,15 @@ show_logging_filename (struct ui_file *file, int from_tty,
 }
 
 static int logging_overwrite;
+
+static void
+set_logging_overwrite (char *args, int from_tty, struct cmd_list_element *c)
+{
+  if (saved_filename)
+    warning (_("Currently logging to %s.  Turn the logging off and on to "
+	       "make the new setting effective."), saved_filename);
+}
+
 static void
 show_logging_overwrite (struct ui_file *file, int from_tty,
 			struct cmd_list_element *c, const char *value)
@@ -55,7 +65,67 @@ Whether logging overwrites or appends to the log file is %s.\n"),
 		    value);
 }
 
+/* Value as configured by the user.  */
 static int logging_redirect;
+
+/* The on-disk file in use if logging is currently active together with
+   redirection turned off (and therefore using tee_file_new).  For active
+   logging with redirection the on-disk file is directly in GDB_STDOUT and
+   this variable is NULL.  */
+static struct ui_file *logging_no_redirect_file;
+
+static void
+set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
+{
+  struct cleanup *cleanups = NULL;
+  struct ui_file *output, *new_logging_no_redirect_file;
+
+  if (saved_filename == NULL
+      || (logging_redirect != 0 && logging_no_redirect_file == NULL)
+      || (logging_redirect == 0 && logging_no_redirect_file != NULL))
+    return;
+
+  if (logging_redirect != 0)
+    {
+      gdb_assert (logging_no_redirect_file != NULL);
+
+      /* ui_out_redirect still has not been called for next gdb_stdout.  */
+      cleanups = make_cleanup_ui_file_delete (gdb_stdout);
+
+      output = logging_no_redirect_file;
+      new_logging_no_redirect_file = NULL;
+
+      if (from_tty)
+	fprintf_unfiltered (saved_output.out, "Redirecting output to %s.\n",
+			    logging_filename);
+    }
+  else
+    {
+      gdb_assert (logging_no_redirect_file == NULL);
+      output = tee_file_new (saved_output.out, 0, gdb_stdout, 0);
+      if (output == NULL)
+	perror_with_name (_("set logging"));
+      new_logging_no_redirect_file = gdb_stdout;
+
+      if (from_tty)
+	fprintf_unfiltered (saved_output.out, "Copying output to %s.\n",
+			    logging_filename);
+    }
+
+  gdb_stdout = output;
+  gdb_stderr = output;
+  gdb_stdlog = output;
+  gdb_stdtarg = output;
+  logging_no_redirect_file = new_logging_no_redirect_file;
+
+  /* It should not happen, the redirection has been already setup.  */
+  if (ui_out_redirect (uiout, output) < 0)
+    warning (_("Current output protocol does not support redirection"));
+
+  if (logging_redirect != 0)
+    do_cleanups (cleanups);
+}
+
 static void
 show_logging_redirect (struct ui_file *file, int from_tty,
 		       struct cmd_list_element *c, const char *value)
@@ -70,6 +140,11 @@ pop_output_files (void)
   /* Only delete one of the files -- they are all set to the same
      value.  */
   ui_file_delete (gdb_stdout);
+  if (logging_no_redirect_file)
+    {
+      ui_file_delete (logging_no_redirect_file);
+      logging_no_redirect_file = NULL;
+    }
   gdb_stdout = saved_output.out;
   gdb_stderr = saved_output.err;
   gdb_stdlog = saved_output.log;
@@ -104,18 +179,25 @@ handle_redirections (int from_tty)
   /* Redirects everything to gdb_stdout while this is running.  */
   if (!logging_redirect)
     {
-      output = tee_file_new (gdb_stdout, 0, output, 1);
+      struct ui_file *no_redirect_file = output;
+
+      output = tee_file_new (gdb_stdout, 0, no_redirect_file, 0);
       if (output == NULL)
 	perror_with_name (_("set logging"));
-      discard_cleanups (cleanups);
-      cleanups = make_cleanup_ui_file_delete (output);
+      make_cleanup_ui_file_delete (output);
       if (from_tty)
 	fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
 			    logging_filename);
+      logging_no_redirect_file = no_redirect_file;
+    }
+  else
+    {
+      gdb_assert (logging_no_redirect_file == NULL);
+
+      if (from_tty)
+	fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
+			    logging_filename);
     }
-  else if (from_tty)
-    fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
-			logging_filename);
 
   discard_cleanups (cleanups);
 
@@ -187,10 +269,20 @@ show_logging_command (char *args, int from_tty)
   else
     printf_unfiltered (_("Logs will be appended to the log file.\n"));
 
-  if (logging_redirect)
-    printf_unfiltered (_("Output will be sent only to the log file.\n"));
+  if (saved_filename)
+    {
+      if (logging_redirect)
+	printf_unfiltered (_("Output is being sent only to the log file.\n"));
+      else
+	printf_unfiltered (_("Output is being logged and displayed.\n"));
+    }
   else
-    printf_unfiltered (_("Output will be logged and displayed.\n"));
+    {
+      if (logging_redirect)
+	printf_unfiltered (_("Output will be sent only to the log file.\n"));
+      else
+	printf_unfiltered (_("Output will be logged and displayed.\n"));
+    }
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
@@ -211,7 +303,7 @@ _initialize_cli_logging (void)
 Set whether logging overwrites or appends to the log file."), _("\
 Show whether logging overwrites or appends to the log file."), _("\
 If set, logging overrides the log file."),
-			   NULL,
+			   set_logging_overwrite,
 			   show_logging_overwrite,
 			   &set_logging_cmdlist, &show_logging_cmdlist);
   add_setshow_boolean_cmd ("redirect", class_support, &logging_redirect, _("\
@@ -219,7 +311,7 @@ Set the logging output mode."), _("\
 Show the logging output mode."), _("\
 If redirect is off, output will go to both the screen and the log file.\n\
 If redirect is on, output will go only to the log file."),
-			   NULL,
+			   set_logging_redirect,
 			   show_logging_redirect,
 			   &set_logging_cmdlist, &show_logging_cmdlist);
   add_setshow_filename_cmd ("file", class_support, &logging_filename, _("\

Re: ping: [patch] set logging {redirect|overwrite} warning

[Pedro Alves]

Sorry, I thought I had already replied to this, but apparently I
dreamt it only.

On Thursday 02 September 2010 15:53:46, Jan Kratochvil wrote:

> No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.
> 
> OK to check-in?

Okay, thanks.

> 2010-08-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* cli-logging.c: Include gdb_assert.h.
> 	(set_logging_overwrite): New function.
> 	(logging_redirect): New comment.
> 	(logging_no_redirect_file, set_logging_redirect)
> 	(pop_output_files) <logging_no_redirect_file>: New.
> 	(handle_redirections) <!logging_redirect>: New variable
> 	no_redirect_file.  Remove file autoclose for tee_file_new.  No longer
> 	discard cleanup for the close of former OUTPUT.  Set
> 	LOGGING_NO_REDIRECT_FILE.
> 	(handle_redirections) <logging_redirect>: gdb_assert
> 	LOGGING_NO_REDIRECT_FILE.
> 	(show_logging_command) <logging_redirect handling>: Adjust messages
> 	for SAVED_FILENAME not NULL.
> 	(_initialize_cli_logging): Install set_logging_overwrite and
> 	set_logging_redirect.

-- 
Pedro Alves

Re: ping: [patch] set logging {redirect|overwrite} warning

[Jan Kratochvil]

On Thu, 02 Sep 2010 17:11:20 +0200, Pedro Alves wrote:
> On Thursday 02 September 2010 15:53:46, Jan Kratochvil wrote:
> > No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.
> > 
> > OK to check-in?
> 
> Okay, thanks.
> 
> > 2010-08-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* cli-logging.c: Include gdb_assert.h.
> > 	(set_logging_overwrite): New function.
> > 	(logging_redirect): New comment.
> > 	(logging_no_redirect_file, set_logging_redirect)
> > 	(pop_output_files) <logging_no_redirect_file>: New.
> > 	(handle_redirections) <!logging_redirect>: New variable
> > 	no_redirect_file.  Remove file autoclose for tee_file_new.  No longer
> > 	discard cleanup for the close of former OUTPUT.  Set
> > 	LOGGING_NO_REDIRECT_FILE.
> > 	(handle_redirections) <logging_redirect>: gdb_assert
> > 	LOGGING_NO_REDIRECT_FILE.
> > 	(show_logging_command) <logging_redirect handling>: Adjust messages
> > 	for SAVED_FILENAME not NULL.
> > 	(_initialize_cli_logging): Install set_logging_overwrite and
> > 	set_logging_redirect.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-09/msg00020.html


Thanks,
Jan