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

Pedro Alves
2015-01-12T19:05:57Z

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