RFA: cleanups -vs- gdb_fopen

RFA: cleanups -vs- gdb_fopen

[Tom Tromey]

This patch changes a few gdb_fopen leaks to use cleanups.  This is
similar to yesterday's patch for fopen.  Basically, there are some
situations where an error could leave an open file descriptor.

In some of these cases, a cleanup might not strictly be required.
Some of the guarded code can only call internal_error, not error.
However, I think it is safer to use cleanups, because it means the
caller is protected against changes in any callee.

Built & regtested on x86-64 (compile farm).

Ok?

Tom

2008-10-28  Tom Tromey  <tromey@redhat.com>

	* cli/cli-logging.c (handle_redirections): Make a cleanup.
	* reggroups.c (maintenance_print_reggroups): Make a cleanup.
	* regcache.c (regcache_print): Make a cleanup.
	* maint.c (maintenance_print_architecture): Make a cleanup.
	* dummy-frame.c (maintenance_print_dummy_frames): Make a cleanup.

diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index 86f1bc0..5f1bcb8 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -85,6 +85,7 @@ pop_output_files (void)
 static void
 handle_redirections (int from_tty)
 {
+  struct cleanup *cleanups;
   struct ui_file *output;
 
   if (saved_filename != NULL)
@@ -97,6 +98,7 @@ handle_redirections (int from_tty)
   output = gdb_fopen (logging_filename, logging_overwrite ? "w" : "a");
   if (output == NULL)
     perror_with_name (_("set logging"));
+  cleanups = make_cleanup_ui_file_delete (output);
 
   /* Redirects everything to gdb_stdout while this is running.  */
   if (!logging_redirect)
@@ -112,6 +114,8 @@ handle_redirections (int from_tty)
     fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
 			logging_filename);
 
+  discard_cleanups (cleanups);
+
   saved_filename = xstrdup (logging_filename);
   saved_output.out = gdb_stdout;
   saved_output.err = gdb_stderr;
diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index a27de2e..9cc3da7 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -265,11 +265,13 @@ maintenance_print_dummy_frames (char *args, int from_tty)
     fprint_dummy_frames (gdb_stdout);
   else
     {
+      struct cleanup *cleanups;
       struct ui_file *file = gdb_fopen (args, "w");
       if (file == NULL)
 	perror_with_name (_("maintenance print dummy-frames"));
+      cleanups = make_cleanup_ui_file_delete (file);
       fprint_dummy_frames (file);    
-      ui_file_delete (file);
+      do_cleanups (cleanups);
     }
 }
 
diff --git a/gdb/maint.c b/gdb/maint.c
index e64d4fe..365e374 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -414,11 +414,13 @@ maintenance_print_architecture (char *args, int from_tty)
     gdbarch_dump (current_gdbarch, gdb_stdout);
   else
     {
+      struct cleanup *cleanups;
       struct ui_file *file = gdb_fopen (args, "w");
       if (file == NULL)
 	perror_with_name (_("maintenance print architecture"));
+      cleanups = make_cleanup_ui_file_delete (file);
       gdbarch_dump (current_gdbarch, file);    
-      ui_file_delete (file);
+      do_cleanups (cleanups);
     }
 }
 
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 616a6f7..74ca6f0 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1104,11 +1104,13 @@ regcache_print (char *args, enum regcache_dump_what what_to_dump)
     regcache_dump (get_current_regcache (), gdb_stdout, what_to_dump);
   else
     {
+      struct cleanup *cleanups;
       struct ui_file *file = gdb_fopen (args, "w");
       if (file == NULL)
 	perror_with_name (_("maintenance print architecture"));
+      cleanups = make_cleanup_ui_file_delete (file);
       regcache_dump (get_current_regcache (), file, what_to_dump);
-      ui_file_delete (file);
+      do_cleanups (cleanups);
     }
 }
 
diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index ea2451e..a4e1d31 100644
--- a/gdb/reggroups.c
+++ b/gdb/reggroups.c
@@ -234,11 +234,13 @@ maintenance_print_reggroups (char *args, int from_tty)
     reggroups_dump (current_gdbarch, gdb_stdout);
   else
     {
+      struct cleanup *cleanups;
       struct ui_file *file = gdb_fopen (args, "w");
       if (file == NULL)
 	perror_with_name (_("maintenance print reggroups"));
+      cleanups = make_cleanup_ui_file_delete (file);
       reggroups_dump (current_gdbarch, file);    
-      ui_file_delete (file);
+      do_cleanups (cleanups);
     }
 }
 

Re: RFA: cleanups -vs- gdb_fopen

[Pedro Alves]

On Tuesday 28 October 2008 18:16:24, Tom Tromey wrote:
> @@ -97,6 +98,7 @@ handle_redirections (int from_tty)
>    output = gdb_fopen (logging_filename, logging_overwrite ? "w" : "a");
>    if (output == NULL)
>      perror_with_name (_("set logging"));
> +  cleanups = make_cleanup_ui_file_delete (output);
>  
>    /* Redirects everything to gdb_stdout while this is running.  */
>    if (!logging_redirect)
> @@ -112,6 +114,8 @@ handle_redirections (int from_tty)
>      fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
>                         logging_filename);
>  
> +  discard_cleanups (cleanups);

This one had me scratch my head a bit:

  output = gdb_fopen (logging_filename, logging_overwrite ? "w" : "a");
  if (output == NULL)
    perror_with_name (_("set logging"));

+  cleanups = make_cleanup_ui_file_delete (output);

  /* Redirects everything to gdb_stdout while this is running.  */
  if (!logging_redirect)
    {
      output = tee_file_new (gdb_stdout, 0, output, 1);
      ^^^^^^                                        ^
      if (output == NULL)
	perror_with_name (_("set logging"));
      if (from_tty)
	fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
			    logging_filename);
    }
  else if (from_tty)
    fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
			logging_filename);

+  discard_cleanups (cleanups);

If an error is thrown after tee_file_new returning a valid file (it
always does currently), you'd run the cleanup on the old `output',
and leave the new `output' leaking.  That can't happen with the current
code, so, your patch looks good to me.

Thanks.

-- 
Pedro Alves

Re: RFA: cleanups -vs- gdb_fopen

[Pedro Alves]

On Thursday 30 October 2008 20:22:06, Tom Tromey wrote:
> What do you think of this variant, which fixes the cleanup after
> tee_file_new?

Thanks!  This is OK.

-- 
Pedro Alves

Re: RFA: cleanups -vs- gdb_fopen

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> This one had me scratch my head a bit:
[...]

Pedro> If an error is thrown after tee_file_new returning a valid file (it
Pedro> always does currently), you'd run the cleanup on the old `output',
Pedro> and leave the new `output' leaking.  That can't happen with the current
Pedro> code, so, your patch looks good to me.

Oops, I missed that.

I think it is nice to try to be safe about this sort of thing.  That
way people don't have to think too hard when making changes between
opening a file and closing it.

What do you think of this variant, which fixes the cleanup after
tee_file_new?

Tom

2008-10-28  Tom Tromey  <tromey@redhat.com>

	* cli/cli-logging.c (handle_redirections): Make a cleanup.
	* reggroups.c (maintenance_print_reggroups): Make a cleanup.
	* regcache.c (regcache_print): Make a cleanup.
	* maint.c (maintenance_print_architecture): Make a cleanup.
	* dummy-frame.c (maintenance_print_dummy_frames): Make a cleanup.

diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index 86f1bc0..1e941b1 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -85,6 +85,7 @@ pop_output_files (void)
 static void
 handle_redirections (int from_tty)
 {
+  struct cleanup *cleanups;
   struct ui_file *output;
 
   if (saved_filename != NULL)
@@ -97,6 +98,7 @@ handle_redirections (int from_tty)
   output = gdb_fopen (logging_filename, logging_overwrite ? "w" : "a");
   if (output == NULL)
     perror_with_name (_("set logging"));
+  cleanups = make_cleanup_ui_file_delete (output);
 
   /* Redirects everything to gdb_stdout while this is running.  */
   if (!logging_redirect)
@@ -104,6 +106,8 @@ handle_redirections (int from_tty)
       output = tee_file_new (gdb_stdout, 0, output, 1);
       if (output == NULL)
 	perror_with_name (_("set logging"));
+      discard_cleanups (cleanups);
+      cleanups = make_cleanup_ui_file_delete (output);
       if (from_tty)
 	fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
 			    logging_filename);
@@ -112,6 +116,8 @@ handle_redirections (int from_tty)
     fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
 			logging_filename);
 
+  discard_cleanups (cleanups);
+
   saved_filename = xstrdup (logging_filename);
   saved_output.out = gdb_stdout;
   saved_output.err = gdb_stderr;
diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index a27de2e..9cc3da7 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -265,11 +265,13 @@ maintenance_print_dummy_frames (char *args, int from_tty)
     fprint_dummy_frames (gdb_stdout);
   else
     {
+      struct cleanup *cleanups;
       struct ui_file *file = gdb_fopen (args, "w");
       if (file == NULL)
 	perror_with_name (_("maintenance print dummy-frames"));
+      cleanups = make_cleanup_ui_file_delete (file);
       fprint_dummy_frames (file);    
-      ui_file_delete (file);
+      do_cleanups (cleanups);
     }
 }
 
diff --git a/gdb/maint.c b/gdb/maint.c
index e64d4fe..365e374 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -414,11 +414,13 @@ maintenance_print_architecture (char *args, int from_tty)
     gdbarch_dump (current_gdbarch, gdb_stdout);
   else
     {
+      struct cleanup *cleanups;
       struct ui_file *file = gdb_fopen (args, "w");
       if (file == NULL)
 	perror_with_name (_("maintenance print architecture"));
+      cleanups = make_cleanup_ui_file_delete (file);
       gdbarch_dump (current_gdbarch, file);    
-      ui_file_delete (file);
+      do_cleanups (cleanups);
     }
 }
 
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 616a6f7..74ca6f0 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1104,11 +1104,13 @@ regcache_print (char *args, enum regcache_dump_what what_to_dump)
     regcache_dump (get_current_regcache (), gdb_stdout, what_to_dump);
   else
     {
+      struct cleanup *cleanups;
       struct ui_file *file = gdb_fopen (args, "w");
       if (file == NULL)
 	perror_with_name (_("maintenance print architecture"));
+      cleanups = make_cleanup_ui_file_delete (file);
       regcache_dump (get_current_regcache (), file, what_to_dump);
-      ui_file_delete (file);
+      do_cleanups (cleanups);
     }
 }
 
diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index ea2451e..a4e1d31 100644
--- a/gdb/reggroups.c
+++ b/gdb/reggroups.c
@@ -234,11 +234,13 @@ maintenance_print_reggroups (char *args, int from_tty)
     reggroups_dump (current_gdbarch, gdb_stdout);
   else
     {
+      struct cleanup *cleanups;
       struct ui_file *file = gdb_fopen (args, "w");
       if (file == NULL)
 	perror_with_name (_("maintenance print reggroups"));
+      cleanups = make_cleanup_ui_file_delete (file);
       reggroups_dump (current_gdbarch, file);    
-      ui_file_delete (file);
+      do_cleanups (cleanups);
     }
 }