svn commit: r330663 - head/sys/kern

Bruce Evans brde at optusnet.com.au
Fri Mar 9 04:42:17 UTC 2018


On Thu, 8 Mar 2018, Mark Johnston wrote:

> Log:
>  Return E2BIG if we run out of space writing a compressed kernel dump.

E2BIG a very wrong errno.  It means "Argment list too long".  It is broken
as designed, with "too" encrypted as "2" and no indication of what is too
big.  EFBIG is not so wrong.  It means "File too large".

>  ENOSPC causes the MD kernel dump code to retry the dump, but this is
>  undesirable in the case where we legitimately ran out of space.

ENOSPC is the correct errno.  It means "[really] No space left on device".
The bug was either retrying or possibly abusing ENOSPC instead of EAGAIN
to mean "transiently out of space for something".

>
> Modified:
>  head/sys/kern/kern_shutdown.c
>
> Modified: head/sys/kern/kern_shutdown.c
> ==============================================================================
> --- head/sys/kern/kern_shutdown.c	Thu Mar  8 16:27:31 2018	(r330662)
> +++ head/sys/kern/kern_shutdown.c	Thu Mar  8 17:04:36 2018	(r330663)
> @@ -1115,6 +1115,12 @@ dump_check_bounds(struct dumperinfo *di, off_t offset,
>
> 	if (length != 0 && (offset < di->mediaoffset ||
> 	    offset - di->mediaoffset + length > di->mediasize)) {
> +		if (di->kdcomp != NULL && offset >= di->mediaoffset) {
> +			printf(
> +		    "Compressed dump failed to fit in device boundaries.\n");
> +			return (E2BIG);
> +		}
> +

Style bug: extra blank line.  Even the outer if statements in this function
are missing this error.

> 		printf("Attempt to write outside dump device boundaries.\n"
> 	    "offset(%jd), mediaoffset(%jd), length(%ju), mediasize(%jd).\n",
> 		    (intmax_t)offset, (intmax_t)di->mediaoffset,

ENOSPC is still returned for this error.  The problem is that this function
shouldn't decide the caller's retry policy or error handling policy.  It
should just return ENOSPC or EINVAL without printing anything.  Then the
caller may retry for ENOSPC if it has alternative method(s) that take less
space.  Then the caller should print the failure message if nothing works.

Other style bugs:
- capitalization of error messages
- termination of error messages with a period
- no prefix for the error messages.
Normal kernel error messages look like "dump: No space left on device".
Here the error messages are English sentences which have to be parsed to
find the subsystem that generated the error.  Since the error messages here
are fairly well written sentences, this is possible, but it often saves
space to not write full sentences and it is always clearer to start with
a prefix.  Here you had to outdent the messages to get them to fit.  Use
of the err() family in userland gives a more uniform style in which it
is impossible to terminate the message with a period (since err() prints
the final word in sterror(errno) and doesn't print a period after it, but
prints a newline so you can't print the period after it).

Bruce


More information about the svn-src-head mailing list