From owner-svn-src-all@freebsd.org Thu Sep 15 05:07:57 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B52F2BDB886; Thu, 15 Sep 2016 05:07:57 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 635E311AC; Thu, 15 Sep 2016 05:07:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id A0D407826A9; Thu, 15 Sep 2016 15:07:53 +1000 (AEST) Date: Thu, 15 Sep 2016 15:07:53 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ed Maste cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r305821 - head/usr.bin/login In-Reply-To: <201609150155.u8F1tI7j029630@repo.freebsd.org> Message-ID: <20160915144746.X806@besplex.bde.org> References: <201609150155.u8F1tI7j029630@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=CoZCCSMD c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=YKd6FehL3BivA6oiOWAA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Sep 2016 05:07:57 -0000 On Thu, 15 Sep 2016, Ed Maste wrote: > Log: > login: clean up errx strings > > errx() prefixes the error string with argv[0] so including "login: " > in the string is redundant. Also remove a superfluous newline. The strings still have plenty of dirt: > Modified: head/usr.bin/login/login_audit.c > ============================================================================== > --- head/usr.bin/login/login_audit.c Wed Sep 14 23:24:23 2016 (r305820) > +++ head/usr.bin/login/login_audit.c Thu Sep 15 01:55:18 2016 (r305821) > @@ -73,14 +73,14 @@ au_login_success(void) > if (auditon(A_GETCOND, &au_cond, sizeof(au_cond)) < 0) { > if (errno == ENOSYS) > return; > - errx(1, "login: Could not determine audit condition"); > + errx(1, "Could not determine audit condition"); Capitalization. Error messages are conventionally not capitalized (but strerror() strings break this). This file does this in many more places, but not all. > @@ -88,22 +88,22 @@ au_login_success(void) > bcopy(&tid, &auinfo.ai_termid, sizeof(auinfo.ai_termid)); > bcopy(&aumask, &auinfo.ai_mask, sizeof(auinfo.ai_mask)); > if (setaudit(&auinfo) != 0) > - err(1, "login: setaudit failed"); > + err(1, "setaudit failed"); Example of normal style for an error message. Should possibly indicate that it was a specific function/syscall that failed by marking up the function with "()". > > if ((aufd = au_open()) == -1) > - errx(1,"login: Audit Error: au_open() failed"); > + errx(1, "Audit Error: au_open() failed"); This marks up the function and adds the doubly Capitalized spam "Audit Error: ". Who would have thought that an error message is for an error? "Audit " is not as redundant as that, but is not used in all of the messages. Since this uses errx() and not err(), a different style is justified, but it is probably wrong to not use strerror() when a function/syscall name is printed. The "failed" part of the message is nondescript. > > if ((tok = au_to_subject32(uid, geteuid(), getegid(), uid, gid, pid, > pid, &tid)) == NULL) > - errx(1, "login: Audit Error: au_to_subject32() failed"); > + errx(1, "Audit Error: au_to_subject32() failed"); > au_write(aufd, tok); > > if ((tok = au_to_return32(0, 0)) == NULL) > - errx(1, "login: Audit Error: au_to_return32() failed"); > + errx(1, "Audit Error: au_to_return32() failed"); > au_write(aufd, tok); Spam continues. The function name is a bit too long to print. > > if (au_close(aufd, 1, AUE_login) == -1) > - errx(1, "login: Audit Record was not committed."); > + errx(1, "Audit Record was not committed."); > } Now "Audit Record" is useful because the function name is not printed. Not printing the function name is a different and perhaps better style. I usually print the function name when I don't want to think about a better discription. Error messages are conventionally not capitalized, and most in this file are not. but this one is. > > /* > @@ -124,13 +124,13 @@ au_login_fail(const char *errmsg, int na > if (auditon(A_GETCOND, &au_cond, sizeof(au_cond)) < 0) { > if (errno == ENOSYS) > return; > - errx(1, "login: Could not determine audit condition"); > + errx(1, "Could not determine audit condition"); > } Back to only 1 capitialization error. > if (au_cond == AUC_NOAUDIT) > return; > > if ((aufd = au_open()) == -1) > - errx(1, "login: Audit Error: au_open() failed"); > + errx(1, "Audit Error: au_open() failed"); Back to spam. > > if (na) { > /* > @@ -139,28 +139,28 @@ au_login_fail(const char *errmsg, int na > */ > if ((tok = au_to_subject32(-1, geteuid(), getegid(), -1, -1, > pid, -1, &tid)) == NULL) > - errx(1, "login: Audit Error: au_to_subject32() failed"); > + errx(1, "Audit Error: au_to_subject32() failed"); > } else { > /* We know the subject -- so use its value instead. */ > uid = pwd->pw_uid; > gid = pwd->pw_gid; > if ((tok = au_to_subject32(uid, geteuid(), getegid(), uid, > gid, pid, pid, &tid)) == NULL) > - errx(1, "login: Audit Error: au_to_subject32() failed"); > + errx(1, "Audit Error: au_to_subject32() failed"); > ... The general style reduces to "error: xxx failed [duh]" with careful use of errx() instead of err() to prevent leakage of useful information. Bruce