From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16031 invoked by alias); 10 Dec 2014 16:54:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 16018 invoked by uid 89); 10 Dec 2014 16:54:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 10 Dec 2014 16:54:18 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sBAGsFxL013108 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 10 Dec 2014 11:54:15 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sBAGsDAa016216; Wed, 10 Dec 2014 11:54:14 -0500 Message-ID: <54887AB5.3000101@redhat.com> Date: Wed, 10 Dec 2014 16:54:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Patrick Palka CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Append to input history file instead of overwriting it References: <1417226462-11254-1-git-send-email-patrick@parcs.ath.cx> <54808956.9070507@redhat.com> <54818CCE.5010701@redhat.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-12/txt/msg00208.txt.bz2 On 12/05/2014 02:11 PM, Patrick Palka wrote: > On Fri, Dec 5, 2014 at 5:45 AM, Pedro Alves 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. > >> 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