Append to input history file instead of overwriting it

  v2: Emit a warning if the first call to rename() fails with errno !=
      ENOENT.
    Don't emit a warning if the second call to rename() fails with
      EEXIST.

This patch makes readline append new history lines to the GDB history
file on exit instead of overwriting the entire history file on exit.
This change allows us to run multiple simultaneous GDB sessions without
having each session overwrite the added history of each other session on
exit.

Care must be taken to ensure that the history file doesn't get corrupted
when multiple GDB processes are trying to simultaneously append to and
then truncate it.  Safety is achieved in such a situation by using an
intermediate local history file to mutually exclude multiple processes
from simultaneously performing write operations on the global history
file.

gdb/ChangeLog:

	* top.h (gdb_add_history): Declare.
	* top.c (command_count): New variable.
	(gdb_add_history): New function.
	(gdb_safe_append_history): New static function.
	(quit_force): Call it.
	(command_line_input): Use gdb_add_history instead of
	add_history.
	* event-top.c (command_line_handler): Likewise.
---
 gdb/event-top.c |  2 +-
 gdb/top.c       | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 gdb/top.h       |  2 ++
 3 files changed, 65 insertions(+), 4 deletions(-)
  

On 01/10/2015 06:46 PM, Patrick Palka wrote:

> +/* Safely append new history entries to the history file in a corruption-free
> +   way using an intermediate local history file.  */
> +
> +static void
> +gdb_safe_append_history (void)
> +{
> +  int ret, saved_errno;
> +  char *local_history_filename;
> +
> +  local_history_filename = xstrprintf ("%s.%d", history_filename, getpid ());

IMO just appending a number is not sufficiently distinct
from what a user might reasonably want to name alternate history
files.  How about picking a more obscure name, like what
I had originally suggested:

  local_history_filename = xstrprintf ("%s-gdb%d~", history_filename, getpid ());

?

> +
> +  ret = rename (history_filename, local_history_filename);
> +  saved_errno = errno;
> +  if (ret < 0 && saved_errno == ENOENT)
> +    {
> +      /* If the rename failed with ENOENT then either the global history file
> +	 never existed in the first place or another GDB process is currently
> +	 appending to it (and has thus temporarily renamed it).  Since we can't
> +	 distinguish between these two cases, we have to conservatively assume
> +	 the first case and therefore must write out (not append) our known
> +	 history to our local history file and try to move it back anyway.
> +	 Otherwise a global history file would never get created!  */
> +      write_history (local_history_filename);
> +    }
> +  else if (ret < 0)
> +    {
> +      warning (_("Could not rename %s to %s: %s"),
> +	       history_filename, local_history_filename, strerror (saved_errno));

use safe_strerror.  Watch out for line too long.


> +      goto out;

Let's avoid gotos when simple enough.  In this case, seems to
me that to skip the second rename call, we only need to move
the "else if" and "else" within a parent "else" branch.

Also, please use a cleanup instead of the xfree at the end:

  local_history_filename = xstrprintf ("%s-gdb%d~", history_filename, getpid ());
  old_chain = make_cleanup (xfree, local_history_filename);
  ...
  do_cleanups (old_chain);

> +    }
> +  else
> +    {
> +      append_history (command_count, local_history_filename);
> +      history_truncate_file (local_history_filename, history_max_entries);
> +    }
> +
> +  ret = rename (local_history_filename, history_filename);
> +  saved_errno = errno;
> +  if (ret < 0 && saved_errno != EEXIST)
> +    warning (_("Could not rename %s to %s: %s"),
> +	     local_history_filename, history_filename, strerror (saved_errno));

safe_strerror.

> +
> +out:
> +  xfree (local_history_filename);
> +}
> +

This is OK with these changes.

Thanks!
  

On Mon, Jan 12, 2015 at 2:05 PM, Pedro Alves <palves@redhat.com> wrote:
> On 01/10/2015 06:46 PM, Patrick Palka wrote:
>
>> +/* Safely append new history entries to the history file in a corruption-free
>> +   way using an intermediate local history file.  */
>> +
>> +static void
>> +gdb_safe_append_history (void)
>> +{
>> +  int ret, saved_errno;
>> +  char *local_history_filename;
>> +
>> +  local_history_filename = xstrprintf ("%s.%d", history_filename, getpid ());
>
> IMO just appending a number is not sufficiently distinct
> from what a user might reasonably want to name alternate history
> files.  How about picking a more obscure name, like what
> I had originally suggested:
>
>   local_history_filename = xstrprintf ("%s-gdb%d~", history_filename, getpid ());
>
> ?
>
>> +
>> +  ret = rename (history_filename, local_history_filename);
>> +  saved_errno = errno;
>> +  if (ret < 0 && saved_errno == ENOENT)
>> +    {
>> +      /* If the rename failed with ENOENT then either the global history file
>> +      never existed in the first place or another GDB process is currently
>> +      appending to it (and has thus temporarily renamed it).  Since we can't
>> +      distinguish between these two cases, we have to conservatively assume
>> +      the first case and therefore must write out (not append) our known
>> +      history to our local history file and try to move it back anyway.
>> +      Otherwise a global history file would never get created!  */
>> +      write_history (local_history_filename);
>> +    }
>> +  else if (ret < 0)
>> +    {
>> +      warning (_("Could not rename %s to %s: %s"),
>> +            history_filename, local_history_filename, strerror (saved_errno));
>
> use safe_strerror.  Watch out for line too long.
>
>
>> +      goto out;
>
> Let's avoid gotos when simple enough.  In this case, seems to
> me that to skip the second rename call, we only need to move
> the "else if" and "else" within a parent "else" branch.
>
> Also, please use a cleanup instead of the xfree at the end:
>
>   local_history_filename = xstrprintf ("%s-gdb%d~", history_filename, getpid ());
>   old_chain = make_cleanup (xfree, local_history_filename);
>   ...
>   do_cleanups (old_chain);
>
>> +    }
>> +  else
>> +    {
>> +      append_history (command_count, local_history_filename);
>> +      history_truncate_file (local_history_filename, history_max_entries);
>> +    }
>> +
>> +  ret = rename (local_history_filename, history_filename);
>> +  saved_errno = errno;
>> +  if (ret < 0 && saved_errno != EEXIST)
>> +    warning (_("Could not rename %s to %s: %s"),
>> +          local_history_filename, history_filename, strerror (saved_errno));
>
> safe_strerror.
>
>> +
>> +out:
>> +  xfree (local_history_filename);
>> +}
>> +
>
> This is OK with these changes.

I have made the changes you requested and committed the attached
patch.  I hope I haven't made any oversights.

Thanks for reviewing!

>
> Thanks!
>
> --
> Pedro Alves
>