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

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Sat, Jan 10, 2015 at 9:09 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Wed, Dec 10, 2014 at 11:54 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 12/05/2014 02:11 PM, Patrick Palka wrote:
>>> On Fri, Dec 5, 2014 at 5:45 AM, Pedro Alves <palves@redhat.com> wrote:
>>>> On 12/05/2014 12:19 AM, Patrick Palka wrote:
>>>>> On Thu, 4 Dec 2014, Pedro Alves wrote:
>>>>>
>>>>>> On 11/29/2014 02:01 AM, Patrick Palka wrote:
>>>>>>> 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.  It is particularly helpful when debugging GDB with GDB.
>>>>>>>
>>>>>>> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.
>>>>>>
>>>>>> Does this mean the history file will keep growing forever, rather than
>>>>>> be trimmed by the history size?
>>>>>
>>>>> That it does... my .gdb_history is up to 2200 lines already!
>>>>>
>>>>> Looks like we have to explicitly truncate the history file on exit after
>>>>> appending to it.  Here's v2 of the patch that initializes the static
>>>>> variable command_count, and calls history_truncate_file() appropriately.
>>>>> Does it look OK?
>>>>
>>>> I'd like to hear how does bash (the canonical readline/history user)
>>>> handle this scenario, if at all.
>>>
>>> It looks like bash truncates the history file size whenever the
>>> $HISTFILESIZE variable is changed (which is usually at startup when it
>>> reads ~/.bashrc), not on exit like this patch does.  It doesn't do any
>>> kind of file-level locking for the truncation operation or for
>>> write_history() or append_history().  It writes directly to $HISTFILE.
>>
>> Bah.
>>
>> Is it that hard to do though?  How about temporarily renaming the
>> history file to something that includes gdb's PID (and would not a
>> file name a user would use in practice) while we append
>> to it, and then (atomically) rename it back?  Something like:
>>
>>  #1 - move $HISTFILE -> $HISTFILE-gdb$PID~
>>  #2 - write/append history to $HISTFILE-gdb$PID~
>>  #3 - move $HISTFILE-gdb$PID~ -> $HISTFILE
>>
>> We can then use non-atomic file access at will in #2, as no
>> other process will be writing to that file (unless the user
>> does evil thinks with "set history filename2, but then he deserves
>> what he gets).
>>
>> This way, if two GDB's race writing to the file, then we'll lose the
>> history of one of them, but that's better than ending up with a
>> corrupted file, IMO.
>
> With your renaming method, what to do if the user has no .gdb_history
> file to begin with?  How would you tell the difference between the
> case of having no .gdb_history and the case of another process in the
> middle of writing to the history file (and thus having temporarily
> renamed it)? In either case it looks like the .gdb_history file
> doesn't exist.  But in the first case we want to write a history file
> anyway, and in the second case we want to give up on writing to the
> history file.  But it doesn't seem possible to tell the difference
> between these two cases with your proposed method.

.... therefore we must conservatively assume case #1, that the history
file does not exist, and to write (not append) the process's known
history to the local history file and try to rename it back anyway.

>
>>
>>>
>>>> It seems like we're opening ourselves
>>>> up for more chances of history file corruption, considering that e.g.,
>>>> you may be quitting several gdb's simultaneously (e.g., when SIGTERM
>>>> is sent to all processes).  A single write call with O_APPEND should
>>>> be atomic, but history_do_write uses mmap if available.  And then
>>>> seems like the truncation operation could end up with a broken hist
>>>> file as well.
>>>> ISTM that if we go this path, we should be doing an atomic update:
>>>> create a new file under a parallel name, and then move to final destination.
>>>
>>> history_truncate_file() is definitely not atomic and does not look
>>> obviously thread-safe, but if bash gets away with not doing any kind
>>> of file-level locking, then can't we? :)
>>>
>>>>
>>>>> 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.
>>>>
>>>>> It is particularly helpful when debugging GDB with GDB.
>>>>
>>>> I'd like to have the description of how this helps that use case
>>>> expanded.  I mean, why would you want the history of the top
>>>> level gdb mixed with the inferior gdb's history?  Like, in:
>>>
>>> I don't necessarily want to, but I think such behavior is more useful
>>> than not retaining the inferior gdb's history at all.  Besides I don't
>>> debug GDB that way.
>>>
>>> In one shell I open "gdb", in another I open "gdb -p $(pidof gdb)" and
>>> I execute commands in both processes from time to time.  In such a
>>> scenario, the contents of the history file depends on which gdb
>>> process I close first.  I would rather like to have the history file
>>> retain the commands of both processes.
>>
>> That still sounds odd to me -- the commands issued in the inferior gdb
>> should be of no use to the other gdb, IMO.
>>
>>> This problem is not peculiar
>>> to debugging GDB with GDB, rather it's encountered whenever there are
>>> multiple GDB processes running simultaneously.
>>
>> Yes, that makes more sense.
>>
>>> So I would rather
>>> remove that remark from the commit message ("It is particularly
>>> helpful when debugging GDB with GDB.") because it's not really true.
>>
>> Alright.
>>
>> Thanks,
>> Pedro Alves