This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Append to input history file instead of overwriting it
- From: Pedro Alves <palves at redhat dot com>
- To: Patrick Palka <patrick at parcs dot ath dot cx>, gdb-patches at sourceware dot org
- Date: Mon, 12 Jan 2015 19:05:39 +0000
- Subject: Re: [PATCH] Append to input history file instead of overwriting it
- Authentication-results: sourceware.org; auth=none
- References: <CA+C-WL8OUwmMQxtLRWgNyJAHgdeHS2UAAdZ99WC+mdBJzgUonA at mail dot gmail dot com> <1420915570-5605-1-git-send-email-patrick at parcs dot ath dot cx>
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