Date: Thu, 15 Sep 2016 15:07:53 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ed Maste <emaste@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r305821 - head/usr.bin/login Message-ID: <20160915144746.X806@besplex.bde.org> In-Reply-To: <201609150155.u8F1tI7j029630@repo.freebsd.org> References: <201609150155.u8F1tI7j029630@repo.freebsd.org>
index | next in thread | previous in thread | raw e-mail
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
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160915144746.X806>