From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14471 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: James Y Knight Newsgroups: gmane.linux.lib.musl.general Subject: Re: error.h implementation Date: Tue, 30 Jul 2019 12:03:56 -0400 Message-ID: References: <20190723041615.GB15665@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="000000000000812d5c058ee829fe" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="10614"; mail-complaints-to="usenet@blaine.gmane.org" To: musl@lists.openwall.com Original-X-From: musl-return-14487-gllmg-musl=m.gmane.org@lists.openwall.com Tue Jul 30 18:04:40 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1hsUcB-0002b3-Nd for gllmg-musl@m.gmane.org; Tue, 30 Jul 2019 18:04:39 +0200 Original-Received: (qmail 23985 invoked by uid 550); 30 Jul 2019 16:04:37 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 23964 invoked from network); 30 Jul 2019 16:04:36 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=SaSBifNl9n75uPySjqzQOmrhhypbka90HJcz/bbvsd8=; b=v+qLcDXlhFvPc6nI+mLYFyhlWiqmVL9+qwDd2WPlIto8uxWjcz20e2X8Xdn959K/KV oX2slx60FJx2NGRHS0qlgQXgLTZeknVbQUN2+EXMZW6jmyNwX9FEpeGFUh4Cum6D9YPD o9xL5LtWFvGcs6GFTq3MphldG8WzEQNdAUw8QoT01mpU5It0qVtGA+fNvTgVZv2JbPPc EOq9F2Qyol4X/55Ot5pTbQNSyqRXU9t7VzSiKS0xKwap5I/K7mL79HgEuKavPxqKA34+ lIn+R+VCuj6LcFPr17aKpdv60gvvKl/3ykrmL2iW+jBjcN9OBsRCDDaNgNwH1WSYibEh eiJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=SaSBifNl9n75uPySjqzQOmrhhypbka90HJcz/bbvsd8=; b=g6wjIZtfxqq1dWXiH1En81V0KCoZSiwJ2Czz1jyGus/HY3c3d878/pAX4T+DCzHsqr 4F4Wdg0emHCQx4f3x5+5+IwCFAFXeEfQjkeWcPHp6HatIx9axdUHuw9Jpg044/hkR5+h bayhgmFrxDCkEBG0eooh2CFZKlH5GLZsFKRocmeNgV4kaM2pEEW/prG2lQjI1OLm0+2U 2CucxOS7YNKz5T2ys0FlpdsBarDfBX3pw7lBFcDwb2V2C5JYWRVbg6K3eiRM85ulD36c pPFbAeCe5RobaZoLz7/aGT5UO8son2oWUQSMX2TdycL46lQmfr5EZ1RkXVNvaQUaO5zZ JKOQ== X-Gm-Message-State: APjAAAWLpdRA6cVCwftFmVhmSmVlq2HAS0coWKYr0GwyZISBXDLaqILO UZXiwH+6g2UJIMoooPNdt3Q4FqAYPoWtdizYTsPXgY5sqDK2wA== X-Google-Smtp-Source: APXvYqxXy95Ly+rcXdEhQFlkUZynl0wMuQln3vEXCaZ7p1L64JJDBPw60kqJbbnomJeByPAHQX1/QzVw3Bj9E9jWo4E= X-Received: by 2002:ab0:30c7:: with SMTP id c7mr11296959uam.143.1564502663349; Tue, 30 Jul 2019 09:04:23 -0700 (PDT) In-Reply-To: <20190723041615.GB15665@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14471 Archived-At: --000000000000812d5c058ee829fe Content-Type: text/plain; charset="UTF-8" On Mon, Jul 22, 2019 at 9:16 PM Rich Felker wrote: > > > A question: is the weak_alias stuff required in this case? I'm not > > 100% clear as to when that's required, and when it's not, but I think > > perhaps it's not actually needed here, because nothing else in the > > libc refers to these nonstandard symbols, and the file defines no > > standard symbols? > > Not needed. It would only be needed if they were being used to > implement something else in one of the standards-governed namespaces. Thanks for the confirmation, fixed. > > diff --git a/dynamic.list b/dynamic.list > > index ee0d363b..d8f57b85 100644 > > --- a/dynamic.list > > +++ b/dynamic.list > > @@ -42,4 +42,11 @@ __progname; > > __progname_full; > > > > __stack_chk_guard; > > + > > +error_print_progname; > > +error_message_count; > > +error_one_per_line; > > +__error_print_progname; > > +__error_message_count; > > +__error_one_per_line; > > }; > > This is a rather bleh aspect of adding them. At least three of those > would be gone with the aliases removed. Yeah. It's really rather unfortunate that copy relocations even exist and this list is even needed at all. Sigh. > > +void error(int status, int errnum, const char *fmt, ...); > > +void error_at_line(int status, int errnum, > > + const char *file, unsigned int linenum, > > + const char *fmt, ...); > > We don't use argument names in header files. Also the convention of > aligning continuations of function arguments like this is not > generally used (and not often needed without names taking up lots of > column space), but not objectionable as long as it's aligned right. Fixed. > > +static void errorv(int status, int errnum, > > + const char *file, unsigned int linenum, > > + const char *fmt, va_list ap) { > > But this one isn't done right -- it's mixing tabs and spaces. Tabs > should only be used for indention levels, spaces for alignment. So if > you were aligning a continued line in an already-indented context, the > continuation would start with the same number of tabs as the line > being continued, followed by spaces to align. This is necessary to be > agnostic to the width of tabs, which is an accessibility issue. indeed, sorry about that. In this case, it looks like usually the function signature is generally not wrapped in musl, even when it's rather long (in this case 111 columns)? Assuming that's the desired style, since it's commonly used elsewhere, I've simply unwrapped this line (and the same for the other functions). Digression -- while I agree in the abstract that the indent-vs-alignment distinction is a great idea, it's uncommonly supported by editors, and is invisible when you get it wrong. This means it requires a good deal of human attention. I think it might be worth considering switching to a spaces-only indentation style. > Also, while { on same line is used for ifs/loops, it's not used for > > start of functions; { always goes on its own line here. Fixed. > > > + ++__error_message_count; > > + > > + fflush(stdout); > > + FLOCK(stderr); > > Please avoid using anything from stdio_impl.h or other implementation > internals when implementing nonstandard functionality that can be done > with 100% portable code in terms of the standard functions (e.g. > flockfile). This principle avoids having "junk" interfaces like the > error.h stuff become maintenance burdens by requiring someone to > understand and change them if the internals change. That's a great principle! Fixed. I wonder if it'd be a good idea to shift around the source code layout for a more obvious layering -- putting the portable library code into one subdir and the OS-specific bits into another. > > + if (__error_print_progname) > > + __error_print_progname(); > And since you're calling an application-provided callback, it's > probably even necessary to use flockfile. I'm not sure it's safe to > use the FLOCK() macro when application code may run in the context > holding the lock, and there's not an intent that it be safe. Indeed. > > + fflush(stderr); > > + FUNLOCK(stderr); > > Is there a reason for the fflush? stderr is unbuffered by default, and > presumably if someone makes it buffered, they want it that way for a > reason..? Nope, I agree, this seems superfluous. Removed. :) > > Also, the first fflush, if it's to be done, should be done after > locking, not before. Otherwise you just take and release the lock > twice for no reason. The previous fflush is flushing stdout, before writing to stderr, so I think it doesn't belong within the locked region. The stdout flushing is part of the documented behavior of this function. > > +void __error_at_line(int status, int errnum, > > + const char *file, unsigned int linenum, > > + const char *fmt, ...) { > > + if (error_one_per_line) { > > + if(saved_linenum == linenum && file != NULL && > > + saved_file != NULL && !strcmp(file, saved_file)) > > + return; > > + saved_linenum = linenum; > > + // Assuming that the lifetime of the passed in file name extends > > + // until the next call is rather questionable, but appears to be > > + // the expected semantics. > > + saved_file = file; > > + } > > I'm not sure if this is okay or not. It probably should have been > designed as a macro that passes __FILE__ and __LINE__ opaquely to the > application, so that the callee can assume file points to a literal > with static storage, but of course everything about these functions is > awful and maybe the implied contract is that you're supposed to use it > that way. Yes, this design is awful, and this undocumented requirement is unfortunate. Yet, indeed, this is the implied contract. :( Fortunately, the use of the 'error_one_per_line = 1' feature is rather rare, e.g. see . Attached updated patch. --000000000000812d5c058ee829fe Content-Type: text/x-patch; charset="US-ASCII"; name="0001-Add-support-for-the-glibc-specific-error.h-header.patch" Content-Disposition: attachment; filename="0001-Add-support-for-the-glibc-specific-error.h-header.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_jyq0gpmx0 RnJvbSA5ODgxODFiNzFlZTI0ZGYwYzM0OWEyYzBhZDBhMGU2NTZlZWZlN2U5IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKYW1lcyBZIEtuaWdodCA8anlrbmlnaHRAZ29vZ2xlLmNvbT4K RGF0ZTogVHVlLCAzMCBKdWwgMjAxOSAxMTo1OToyNSAtMDQwMApTdWJqZWN0OiBbUEFUQ0hdIEFk ZCBzdXBwb3J0IGZvciB0aGUgZ2xpYmMtc3BlY2lmaWMgZXJyb3IuaCBoZWFkZXIuCgpUaGlzIGlu Y2x1ZGVzIHRoZSBmdW5jdGlvbnMgJ2Vycm9yJywgJ2Vycm9yX2F0X2xpbmUnLCBhbmQgdGhlaXIK YXNzb2NpYXRlZCBnbG9iYWwgdmFyaWFibGVzLgotLS0KIGR5bmFtaWMubGlzdCAgICAgICB8ICA0 ICsrKwogaW5jbHVkZS9lcnJvci5oICAgIHwgMjEgKysrKysrKysrKysrKysKIHNyYy9sZWdhY3kv ZXJyb3IuYyB8IDY5ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr KysKIDMgZmlsZXMgY2hhbmdlZCwgOTQgaW5zZXJ0aW9ucygrKQogY3JlYXRlIG1vZGUgMTAwNjQ0 IGluY2x1ZGUvZXJyb3IuaAogY3JlYXRlIG1vZGUgMTAwNjQ0IHNyYy9sZWdhY3kvZXJyb3IuYwoK ZGlmZiAtLWdpdCBhL2R5bmFtaWMubGlzdCBiL2R5bmFtaWMubGlzdAppbmRleCBlZTBkMzYzYi4u OWM4NDUwZjQgMTAwNjQ0Ci0tLSBhL2R5bmFtaWMubGlzdAorKysgYi9keW5hbWljLmxpc3QKQEAg LTQyLDQgKzQyLDggQEAgX19wcm9nbmFtZTsKIF9fcHJvZ25hbWVfZnVsbDsKIAogX19zdGFja19j aGtfZ3VhcmQ7CisKK2Vycm9yX3ByaW50X3Byb2duYW1lOworZXJyb3JfbWVzc2FnZV9jb3VudDsK K2Vycm9yX29uZV9wZXJfbGluZTsKIH07CmRpZmYgLS1naXQgYS9pbmNsdWRlL2Vycm9yLmggYi9p bmNsdWRlL2Vycm9yLmgKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAuLjAzYjFj YTQxCi0tLSAvZGV2L251bGwKKysrIGIvaW5jbHVkZS9lcnJvci5oCkBAIC0wLDAgKzEsMjEgQEAK KyNpZm5kZWYgX0VSUk9SX0gKKyNkZWZpbmUgX0VSUk9SX0gKKworI2luY2x1ZGUgPGZlYXR1cmVz Lmg+CisKKyNpZmRlZiBfX2NwbHVzcGx1cworZXh0ZXJuICJDIiB7CisjZW5kaWYKKworZXh0ZXJu IHZvaWQgKCplcnJvcl9wcmludF9wcm9nbmFtZSkgKHZvaWQpOworZXh0ZXJuIHVuc2lnbmVkIGlu dCBlcnJvcl9tZXNzYWdlX2NvdW50OworZXh0ZXJuIGludCBlcnJvcl9vbmVfcGVyX2xpbmU7CisK K3ZvaWQgZXJyb3IoaW50LCBpbnQsIGNvbnN0IGNoYXIgKiwgLi4uKTsKK3ZvaWQgZXJyb3JfYXRf bGluZShpbnQsIGludCwgY29uc3QgY2hhciAqLCB1bnNpZ25lZCBpbnQsIGNvbnN0IGNoYXIgKiwg Li4uKTsKKworI2lmZGVmIF9fY3BsdXNwbHVzCit9CisjZW5kaWYKKworI2VuZGlmCmRpZmYgLS1n aXQgYS9zcmMvbGVnYWN5L2Vycm9yLmMgYi9zcmMvbGVnYWN5L2Vycm9yLmMKbmV3IGZpbGUgbW9k ZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAuLmM1MDAwZmE0Ci0tLSAvZGV2L251bGwKKysrIGIvc3Jj L2xlZ2FjeS9lcnJvci5jCkBAIC0wLDAgKzEsNjkgQEAKKyNpbmNsdWRlIDxlcnJvci5oPgorI2lu Y2x1ZGUgPHN0ZGFyZy5oPgorI2luY2x1ZGUgPHN0ZGlvLmg+CisjaW5jbHVkZSA8c3RkbGliLmg+ CisjaW5jbHVkZSA8c3RyaW5nLmg+CisjaW5jbHVkZSAibGliYy5oIgorCit2b2lkICgqZXJyb3Jf cHJpbnRfcHJvZ25hbWUpICh2b2lkKSA9IDA7Cit1bnNpZ25lZCBpbnQgZXJyb3JfbWVzc2FnZV9j b3VudCA9IDA7CitpbnQgZXJyb3Jfb25lX3Blcl9saW5lID0gMDsKKworc3RhdGljIHVuc2lnbmVk IGludCBzYXZlZF9saW5lbnVtID0gMDsKK3N0YXRpYyBjb25zdCBjaGFyICpzYXZlZF9maWxlID0g MDsKKworc3RhdGljIHZvaWQgZXJyb3J2KGludCBzdGF0dXMsIGludCBlcnJudW0sIGNvbnN0IGNo YXIgKmZpbGUsIHVuc2lnbmVkIGludCBsaW5lbnVtLCBjb25zdCBjaGFyICpmbXQsIHZhX2xpc3Qg YXApCit7CisJKytlcnJvcl9tZXNzYWdlX2NvdW50OworCisJZmZsdXNoKHN0ZG91dCk7CisJZmxv Y2tmaWxlKHN0ZGVycik7CisKKwlpZiAoZXJyb3JfcHJpbnRfcHJvZ25hbWUpCisJCWVycm9yX3By aW50X3Byb2duYW1lKCk7CisJZWxzZSB7CisJCWZwcmludGYoc3RkZXJyLCAiJXM6IiwgX19wcm9n bmFtZV9mdWxsKTsKKwkJaWYgKCFmaWxlKQorCQkJZnB1dGMoJyAnLCBzdGRlcnIpOworCX0KKwor CWlmIChmaWxlKQorCQlmcHJpbnRmKHN0ZGVyciwgIiVzOiV1OiAiLCBmaWxlLCBsaW5lbnVtKTsK KworCXZmcHJpbnRmKHN0ZGVyciwgZm10LCBhcCk7CisJaWYgKGVycm51bSkKKwkJZnByaW50Zihz dGRlcnIsICI6ICVzIiwgc3RyZXJyb3IoZXJybnVtKSk7CisJZnB1dGMoJ1xuJywgc3RkZXJyKTsK KworCWZ1bmxvY2tmaWxlKHN0ZGVycik7CisKKwlpZiAoc3RhdHVzKQorCQlleGl0KHN0YXR1cyk7 Cit9CisKK3ZvaWQgZXJyb3IoaW50IHN0YXR1cywgaW50IGVycm51bSwgY29uc3QgY2hhciAqZm10 LCAuLi4pCit7CisJdmFfbGlzdCBhcDsKKwl2YV9zdGFydChhcCwgZm10KTsKKwllcnJvcnYoc3Rh dHVzLCBlcnJudW0sIE5VTEwsIDAsIGZtdCwgYXApOworCXZhX2VuZChhcCk7Cit9CisKK3ZvaWQg ZXJyb3JfYXRfbGluZShpbnQgc3RhdHVzLCBpbnQgZXJybnVtLCBjb25zdCBjaGFyICpmaWxlLCB1 bnNpZ25lZCBpbnQgbGluZW51bSwgY29uc3QgY2hhciAqZm10LCAuLi4pCit7CisJaWYgKGVycm9y X29uZV9wZXJfbGluZSkgeworCQlpZihzYXZlZF9saW5lbnVtID09IGxpbmVudW0gJiYgZmlsZSAh PSBOVUxMICYmCisJCSAgIHNhdmVkX2ZpbGUgIT0gTlVMTCAmJiAhc3RyY21wKGZpbGUsIHNhdmVk X2ZpbGUpKQorCQkJcmV0dXJuOworCQlzYXZlZF9saW5lbnVtID0gbGluZW51bTsKKwkJLy8gQXNz dW1pbmcgdGhhdCB0aGUgbGlmZXRpbWUgb2YgdGhlIHBhc3NlZCBpbiBmaWxlIG5hbWUgZXh0ZW5k cworCQkvLyB1bnRpbCB0aGUgbmV4dCBjYWxsIGlzIHJhdGhlciBxdWVzdGlvbmFibGUsIGJ1dCBh cHBlYXJzIHRvIGJlCisJCS8vIHRoZSBleHBlY3RlZCBzZW1hbnRpY3MuCisJCXNhdmVkX2ZpbGUg PSBmaWxlOworCX0KKworCXZhX2xpc3QgYXA7CisJdmFfc3RhcnQoYXAsIGZtdCk7CisJZXJyb3J2 KHN0YXR1cywgZXJybnVtLCBmaWxlLCBsaW5lbnVtLCBmbXQsIGFwKTsKKwl2YV9lbmQoYXApOwor fQotLSAKMi4yMi4wLjcwOS5nMTAyMzAyMTQ3Yi1nb29nCgo= --000000000000812d5c058ee829fe--