From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17463 invoked by alias); 12 Jan 2015 22:56:56 -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 17452 invoked by uid 89); 12 Jan 2015 22:56:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-pd0-f176.google.com Received: from mail-pd0-f176.google.com (HELO mail-pd0-f176.google.com) (209.85.192.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 12 Jan 2015 22:56:54 +0000 Received: by mail-pd0-f176.google.com with SMTP id r10so32926830pdi.7 for ; Mon, 12 Jan 2015 14:56:52 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=V11arp7Rfdnh2VaOZItaexGitdGRXXzsvK+iPj6DyPo=; b=mdZie1B0SH03mGiAhlZnA2FwRgMzpo+8RHkXWfnrl0E6ZvtNjOoXb9fhTgoeaCoPly 5jYjXDNKL7WkqUJARYK3efVRLYNcjs82Dxz/YH5l0uxRPXziQ1fVIMSK63N/beYhxSmV 6zbwcFcVNa47uubPlKZBkefhtqv95EXqZM8gWClZAdaYWf6MtkbaJPKOEF6mcqWGh/VM RJM2CqCiDSjR91eKjivtGHDKiTc3+QIHsU5oZBJW+lykX+yIqzbRBnSwiE6e/abS4Ltn bRo8yQGugkyqHmXGTUtG0bpW5VRWl63QY/rjXANCl4amOiMlI0olgbRdKKadQA7Hg3Vf d4mQ== X-Gm-Message-State: ALoCoQkWOkRQyWOjGyGhbwDN4Cvzj3x1J/3tojzRMODkYTd3KPJDawReEDsnQk4oq5vLbYL1I0aU X-Received: by 10.70.131.198 with SMTP id oo6mr47654974pdb.77.1421103412336; Mon, 12 Jan 2015 14:56:52 -0800 (PST) MIME-Version: 1.0 Received: by 10.70.22.145 with HTTP; Mon, 12 Jan 2015 14:56:31 -0800 (PST) In-Reply-To: <54B41B03.5030306@redhat.com> References: <1420915570-5605-1-git-send-email-patrick@parcs.ath.cx> <54B41B03.5030306@redhat.com> From: Patrick Palka Date: Mon, 12 Jan 2015 22:56:00 -0000 Message-ID: Subject: Re: [PATCH] Append to input history file instead of overwriting it To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: multipart/mixed; boundary=001a11332d7c15690d050c7c6ce9 X-SW-Source: 2015-01/txt/msg00323.txt.bz2 --001a11332d7c15690d050c7c6ce9 Content-Type: text/plain; charset=UTF-8 Content-length: 2975 On Mon, Jan 12, 2015 at 2:05 PM, Pedro Alves 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 > --001a11332d7c15690d050c7c6ce9 Content-Type: application/octet-stream; name="0001-Append-to-input-history-file-instead-of-overwriting-.patch" Content-Disposition: attachment; filename="0001-Append-to-input-history-file-instead-of-overwriting-.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_i4ug9ush0 Content-length: 7117 RnJvbSA2MDg2ZDVlNzAxNjNjNjJiZWRmMzIyODIwMGMzNzhhMDRhZjIzNjQ2 IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQYXRyaWNrIFBhbGth IDxwYXRyaWNrQHBhcmNzLmF0aC5jeD4KRGF0ZTogRnJpLCAyOCBOb3YgMjAx NCAyMDoyMjo0NSAtMDUwMApTdWJqZWN0OiBbUEFUQ0hdIEFwcGVuZCB0byBp bnB1dCBoaXN0b3J5IGZpbGUgaW5zdGVhZCBvZiBvdmVyd3JpdGluZyBpdAoK VGhpcyBwYXRjaCBtYWtlcyByZWFkbGluZSBhcHBlbmQgbmV3IGhpc3Rvcnkg bGluZXMgdG8gdGhlIEdEQiBoaXN0b3J5CmZpbGUgb24gZXhpdCBpbnN0ZWFk IG9mIG92ZXJ3cml0aW5nIHRoZSBlbnRpcmUgaGlzdG9yeSBmaWxlIG9uIGV4 aXQuClRoaXMgY2hhbmdlIGFsbG93cyB1cyB0byBydW4gbXVsdGlwbGUgc2lt dWx0YW5lb3VzIEdEQiBzZXNzaW9ucyB3aXRob3V0CmhhdmluZyBlYWNoIHNl c3Npb24gb3ZlcndyaXRlIHRoZSBhZGRlZCBoaXN0b3J5IG9mIGVhY2ggb3Ro ZXIgc2Vzc2lvbiBvbgpleGl0LgoKQ2FyZSBtdXN0IGJlIHRha2VuIHRvIGVu c3VyZSB0aGF0IHRoZSBoaXN0b3J5IGZpbGUgZG9lc24ndCBnZXQgY29ycnVw dGVkCndoZW4gbXVsdGlwbGUgR0RCIHByb2Nlc3NlcyBhcmUgdHJ5aW5nIHRv IHNpbXVsdGFuZW91c2x5IGFwcGVuZCB0byBhbmQKdGhlbiB0cnVuY2F0ZSBp dC4gIFNhZmV0eSBpcyBhY2hpZXZlZCBpbiBzdWNoIGEgc2l0dWF0aW9uIGJ5 IHVzaW5nIGFuCmludGVybWVkaWF0ZSBsb2NhbCBoaXN0b3J5IGZpbGUgdG8g bXV0dWFsbHkgZXhjbHVkZSBtdWx0aXBsZSBwcm9jZXNzZXMKZnJvbSBzaW11 bHRhbmVvdXNseSBwZXJmb3JtaW5nIHdyaXRlIG9wZXJhdGlvbnMgb24gdGhl IGdsb2JhbCBoaXN0b3J5CmZpbGUuCgpnZGIvQ2hhbmdlTG9nOgoKCSogdG9w LmggKGdkYl9hZGRfaGlzdG9yeSk6IERlY2xhcmUuCgkqIHRvcC5jIChjb21t YW5kX2NvdW50KTogTmV3IHZhcmlhYmxlLgoJKGdkYl9hZGRfaGlzdG9yeSk6 IE5ldyBmdW5jdGlvbi4KCShnZGJfc2FmZV9hcHBlbmRfaGlzdG9yeSk6IE5l dyBzdGF0aWMgZnVuY3Rpb24uCgkocXVpdF9mb3JjZSk6IENhbGwgaXQuCgko Y29tbWFuZF9saW5lX2lucHV0KTogVXNlIGdkYl9hZGRfaGlzdG9yeSBpbnN0 ZWFkIG9mCglhZGRfaGlzdG9yeS4KCSogZXZlbnQtdG9wLmMgKGNvbW1hbmRf bGluZV9oYW5kbGVyKTogTGlrZXdpc2UuCi0tLQogZ2RiL2V2ZW50LXRvcC5j IHwgIDIgKy0KIGdkYi90b3AuYyAgICAgICB8IDczICsrKysrKysrKysrKysr KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0tLQog Z2RiL3RvcC5oICAgICAgIHwgIDIgKysKIDMgZmlsZXMgY2hhbmdlZCwgNzMg aW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9n ZGIvZXZlbnQtdG9wLmMgYi9nZGIvZXZlbnQtdG9wLmMKaW5kZXggMTNkZGVl Mi4uYmJkYTVkYyAxMDA2NDQKLS0tIGEvZ2RiL2V2ZW50LXRvcC5jCisrKyBi L2dkYi9ldmVudC10b3AuYwpAQCAtNjY3LDcgKzY2Nyw3IEBAIGNvbW1hbmRf bGluZV9oYW5kbGVyIChjaGFyICpybCkKIAogICAvKiBBZGQgbGluZSB0byBo aXN0b3J5IGlmIGFwcHJvcHJpYXRlLiAgKi8KICAgaWYgKCpsaW5lYnVmZmVy ICYmIGlucHV0X2Zyb21fdGVybWluYWxfcCAoKSkKLSAgICBhZGRfaGlzdG9y eSAobGluZWJ1ZmZlcik7CisgICAgZ2RiX2FkZF9oaXN0b3J5IChsaW5lYnVm ZmVyKTsKIAogICAvKiBOb3RlOiBsaW5lcyBjb25zaXN0aW5nIHNvbGVseSBv ZiBjb21tZW50cyBhcmUgYWRkZWQgdG8gdGhlIGNvbW1hbmQKICAgICAgaGlz dG9yeS4gIFRoaXMgaXMgdXNlZnVsIHdoZW4geW91IHR5cGUgYSBjb21tYW5k LCBhbmQgdGhlbgpkaWZmIC0tZ2l0IGEvZ2RiL3RvcC5jIGIvZ2RiL3RvcC5j CmluZGV4IGI4NWVhMWEuLmExNDYyYTAgMTAwNjQ0Ci0tLSBhL2dkYi90b3Au YworKysgYi9nZGIvdG9wLmMKQEAgLTg5NSw3ICs4OTUsNzQgQEAgZ2RiX3Js X29wZXJhdGVfYW5kX2dldF9uZXh0IChpbnQgY291bnQsIGludCBrZXkpCiAK ICAgcmV0dXJuIHJsX25ld2xpbmUgKDEsIGtleSk7CiB9Ci0MCisKKy8qIE51 bWJlciBvZiB1c2VyIGNvbW1hbmRzIGV4ZWN1dGVkIGR1cmluZyB0aGlzIHNl c3Npb24uICAqLworCitzdGF0aWMgaW50IGNvbW1hbmRfY291bnQgPSAwOwor CisvKiBBZGQgdGhlIHVzZXIgY29tbWFuZCBDT01NQU5EIHRvIHRoZSBpbnB1 dCBoaXN0b3J5IGxpc3QuICAqLworCit2b2lkCitnZGJfYWRkX2hpc3Rvcnkg KGNvbnN0IGNoYXIgKmNvbW1hbmQpCit7CisgIGFkZF9oaXN0b3J5IChjb21t YW5kKTsKKyAgY29tbWFuZF9jb3VudCsrOworfQorCisvKiBTYWZlbHkgYXBw ZW5kIG5ldyBoaXN0b3J5IGVudHJpZXMgdG8gdGhlIGhpc3RvcnkgZmlsZSBp biBhIGNvcnJ1cHRpb24tZnJlZQorICAgd2F5IHVzaW5nIGFuIGludGVybWVk aWF0ZSBsb2NhbCBoaXN0b3J5IGZpbGUuICAqLworCitzdGF0aWMgdm9pZAor Z2RiX3NhZmVfYXBwZW5kX2hpc3RvcnkgKHZvaWQpCit7CisgIGludCByZXQs IHNhdmVkX2Vycm5vOworICBjaGFyICpsb2NhbF9oaXN0b3J5X2ZpbGVuYW1l OworICBzdHJ1Y3QgY2xlYW51cCAqb2xkX2NoYWluOworCisgIGxvY2FsX2hp c3RvcnlfZmlsZW5hbWUKKyAgICA9IHhzdHJwcmludGYgKCIlcy1nZGIlZH4i LCBoaXN0b3J5X2ZpbGVuYW1lLCBnZXRwaWQgKCkpOworICBvbGRfY2hhaW4g PSBtYWtlX2NsZWFudXAgKHhmcmVlLCBsb2NhbF9oaXN0b3J5X2ZpbGVuYW1l KTsKKworICByZXQgPSByZW5hbWUgKGhpc3RvcnlfZmlsZW5hbWUsIGxvY2Fs X2hpc3RvcnlfZmlsZW5hbWUpOworICBzYXZlZF9lcnJubyA9IGVycm5vOwor ICBpZiAocmV0IDwgMCAmJiBzYXZlZF9lcnJubyAhPSBFTk9FTlQpCisgICAg eworICAgICAgd2FybmluZyAoXygiQ291bGQgbm90IHJlbmFtZSAlcyB0byAl czogJXMiKSwKKwkgICAgICAgaGlzdG9yeV9maWxlbmFtZSwgbG9jYWxfaGlz dG9yeV9maWxlbmFtZSwKKwkgICAgICAgc2FmZV9zdHJlcnJvciAoc2F2ZWRf ZXJybm8pKTsKKyAgICB9CisgIGVsc2UKKyAgICB7CisgICAgICBpZiAocmV0 IDwgMCkKKwl7CisJICAvKiBJZiB0aGUgcmVuYW1lIGZhaWxlZCB3aXRoIEVO T0VOVCB0aGVuIGVpdGhlciB0aGUgZ2xvYmFsIGhpc3RvcnkKKwkgICAgIGZp bGUgbmV2ZXIgZXhpc3RlZCBpbiB0aGUgZmlyc3QgcGxhY2Ugb3IgYW5vdGhl ciBHREIgcHJvY2VzcyBpcworCSAgICAgY3VycmVudGx5IGFwcGVuZGluZyB0 byBpdCAoYW5kIGhhcyB0aHVzIHRlbXBvcmFyaWx5IHJlbmFtZWQgaXQpLgor CSAgICAgU2luY2Ugd2UgY2FuJ3QgZGlzdGluZ3Vpc2ggYmV0d2VlbiB0aGVz ZSB0d28gY2FzZXMsIHdlIGhhdmUgdG8KKwkgICAgIGNvbnNlcnZhdGl2ZWx5 IGFzc3VtZSB0aGUgZmlyc3QgY2FzZSBhbmQgdGhlcmVmb3JlIG11c3Qgd3Jp dGUgb3V0CisJICAgICAobm90IGFwcGVuZCkgb3VyIGtub3duIGhpc3Rvcnkg dG8gb3VyIGxvY2FsIGhpc3RvcnkgZmlsZSBhbmQgdHJ5CisJICAgICB0byBt b3ZlIGl0IGJhY2sgYW55d2F5LiAgT3RoZXJ3aXNlIGEgZ2xvYmFsIGhpc3Rv cnkgZmlsZSB3b3VsZAorCSAgICAgbmV2ZXIgZ2V0IGNyZWF0ZWQhICAqLwor CSAgIGdkYl9hc3NlcnQgKHNhdmVkX2Vycm5vID09IEVOT0VOVCk7CisJICAg d3JpdGVfaGlzdG9yeSAobG9jYWxfaGlzdG9yeV9maWxlbmFtZSk7CisJfQor ICAgICAgZWxzZQorCXsKKwkgIGFwcGVuZF9oaXN0b3J5IChjb21tYW5kX2Nv dW50LCBsb2NhbF9oaXN0b3J5X2ZpbGVuYW1lKTsKKwkgIGhpc3RvcnlfdHJ1 bmNhdGVfZmlsZSAobG9jYWxfaGlzdG9yeV9maWxlbmFtZSwgaGlzdG9yeV9t YXhfZW50cmllcyk7CisJfQorCisgICAgICByZXQgPSByZW5hbWUgKGxvY2Fs X2hpc3RvcnlfZmlsZW5hbWUsIGhpc3RvcnlfZmlsZW5hbWUpOworICAgICAg c2F2ZWRfZXJybm8gPSBlcnJubzsKKyAgICAgIGlmIChyZXQgPCAwICYmIHNh dmVkX2Vycm5vICE9IEVFWElTVCkKKyAgICAgICAgd2FybmluZyAoXygiQ291 bGQgbm90IHJlbmFtZSAlcyB0byAlczogJXMiKSwKKwkJIGxvY2FsX2hpc3Rv cnlfZmlsZW5hbWUsIGhpc3RvcnlfZmlsZW5hbWUsCisJCSBzYWZlX3N0cmVy cm9yIChzYXZlZF9lcnJubykpOworICAgIH0KKworICBkb19jbGVhbnVwcyAo b2xkX2NoYWluKTsKK30KKwogLyogUmVhZCBvbmUgbGluZSBmcm9tIHRoZSBj b21tYW5kIGlucHV0IHN0cmVhbSBgaW5zdHJlYW0nCiAgICBpbnRvIHRoZSBs b2NhbCBzdGF0aWMgYnVmZmVyIGBsaW5lYnVmZmVyJyAod2hvc2UgY3VycmVu dCBsZW5ndGgKICAgIGlzIGBsaW5lbGVuZ3RoJykuCkBAIC0xMDk0LDcgKzEx NjEsNyBAQCBjb21tYW5kX2xpbmVfaW5wdXQgKGNvbnN0IGNoYXIgKnByb21w dF9hcmcsIGludCByZXBlYXQsIGNoYXIgKmFubm90YXRpb25fc3VmZml4KQog CiAgIC8qIEFkZCBsaW5lIHRvIGhpc3RvcnkgaWYgYXBwcm9wcmlhdGUuICAq LwogICBpZiAoKmxpbmVidWZmZXIgJiYgaW5wdXRfZnJvbV90ZXJtaW5hbF9w ICgpKQotICAgIGFkZF9oaXN0b3J5IChsaW5lYnVmZmVyKTsKKyAgICBnZGJf YWRkX2hpc3RvcnkgKGxpbmVidWZmZXIpOwogCiAgIC8qIFNhdmUgaW50byBn bG9iYWwgYnVmZmVyIGlmIGFwcHJvcHJpYXRlLiAgKi8KICAgaWYgKHJlcGVh dCkKQEAgLTE0NDUsNyArMTUxMiw3IEBAIHF1aXRfZm9yY2UgKGNoYXIgKmFy Z3MsIGludCBmcm9tX3R0eSkKICAgICB7CiAgICAgICBpZiAod3JpdGVfaGlz dG9yeV9wICYmIGhpc3RvcnlfZmlsZW5hbWUKIAkgICYmIGlucHV0X2Zyb21f dGVybWluYWxfcCAoKSkKLQl3cml0ZV9oaXN0b3J5IChoaXN0b3J5X2ZpbGVu YW1lKTsKKwlnZGJfc2FmZV9hcHBlbmRfaGlzdG9yeSAoKTsKICAgICB9CiAg IERPX1BSSU5UX0VYOwogCmRpZmYgLS1naXQgYS9nZGIvdG9wLmggYi9nZGIv dG9wLmgKaW5kZXggYjY4ZTg5Ni4uOTg3Mjc5YiAxMDA2NDQKLS0tIGEvZ2Ri L3RvcC5oCisrKyBiL2dkYi90b3AuaApAQCAtNzksNiArNzksOCBAQCBleHRl cm4gaW50IGhpc3RvcnlfZXhwYW5zaW9uX3A7CiBleHRlcm4gaW50IHNlcnZl cl9jb21tYW5kOwogZXh0ZXJuIGNoYXIgKmxpbV9hdF9zdGFydDsKIAorZXh0 ZXJuIHZvaWQgZ2RiX2FkZF9oaXN0b3J5IChjb25zdCBjaGFyICopOworCiBl eHRlcm4gdm9pZCBzaG93X2NvbW1hbmRzIChjaGFyICphcmdzLCBpbnQgZnJv bV90dHkpOwogCiBleHRlcm4gdm9pZCBzZXRfaGlzdG9yeSAoY2hhciAqLCBp bnQpOwotLSAKMi4yLjEuMjEyLmdjNWI5MjU2Cgo= --001a11332d7c15690d050c7c6ce9--