Re: [PATCH] Append to input history file instead of overwriting it

[Pedro Alves <palves@redhat.com>]

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!

-- 
Pedro Alves