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
>