From owner-svn-src-head@freebsd.org Fri Mar 9 04:42:16 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id AC8B4F2B012; Fri, 9 Mar 2018 04:42:16 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 2F47A823AB; Fri, 9 Mar 2018 04:42:15 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 2F77F1A2661; Fri, 9 Mar 2018 15:42:06 +1100 (AEDT) Date: Fri, 9 Mar 2018 15:42:05 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mark Johnston cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r330663 - head/sys/kern In-Reply-To: <201803081704.w28H4aQx052056@repo.freebsd.org> Message-ID: <20180309150402.X950@besplex.bde.org> References: <201803081704.w28H4aQx052056@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.2 cv=FNpr/6gs c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=IXfSM6YwnFqPYyAIxbIA:9 a=7064yCuboU82dyd_:21 a=PYkMtr5hiBaTbJBV:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Mar 2018 04:42:17 -0000 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