From owner-svn-src-head@FreeBSD.ORG Fri Jun 1 09:14:21 2012 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 14EFA1065674; Fri, 1 Jun 2012 09:14:21 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail04.syd.optusnet.com.au (mail04.syd.optusnet.com.au [211.29.132.185]) by mx1.freebsd.org (Postfix) with ESMTP id 4FF458FC18; Fri, 1 Jun 2012 09:14:20 +0000 (UTC) Received: from c122-106-171-232.carlnfd1.nsw.optusnet.com.au (c122-106-171-232.carlnfd1.nsw.optusnet.com.au [122.106.171.232]) by mail04.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q519E9Ym032736 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 1 Jun 2012 19:14:11 +1000 Date: Fri, 1 Jun 2012 19:14:09 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Sergey Kandaurov In-Reply-To: Message-ID: <20120601175403.H1865@besplex.bde.org> References: <201206010442.q514gqqv084148@svn.freebsd.org> MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-1047696930-1338542049=:1865" Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler Subject: Re: svn commit: r236380 - head/sys/vm X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 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, 01 Jun 2012 09:14:21 -0000 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-1047696930-1338542049=:1865 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE On Fri, 1 Jun 2012, Sergey Kandaurov wrote: > On 1 June 2012 08:42, Eitan Adler wrote: >> ... >> Log: >> =A0Add sysctl to query amount of swap space free >> >> =A0PR: =A0 =A0 =A0 =A0 =A0 kern/166780 >> =A0Submitted by: Radim Kolar >> =A0Approved by: =A0cperciva >> =A0MFC after: =A0 =A01 week > > Well, we already have more powerful vm.swap_info, so > I see no reason to add yet another one to do the same thing > (but now with a human interface). The new interface provides many more bugs. Mostly style bugs, but also type mismatches and potential overflow. > Probably sysctl(8) should be enhanced to parse it instead. That would be another bug. sysctl(8) already does too much parsing. Parsing belongs in specialized utilities, and I think there are already some that do it for swap. sysctl(8) largest existing exessive parsing and presenting is for related vmtotal things. >> Modified: >> =A0head/sys/vm/swap_pager.c >> >> Modified: head/sys/vm/swap_pager.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D >> --- head/sys/vm/swap_pager.c =A0 =A0Fri Jun =A01 04:34:49 2012 =A0 =A0 = =A0 =A0(r236379) >> +++ head/sys/vm/swap_pager.c =A0 =A0Fri Jun =A01 04:42:52 2012 =A0 =A0 = =A0 =A0(r236380) >> @@ -2692,3 +2692,18 @@ swaponvp(struct thread *td, struct vnode >> =A0 =A0 =A0 =A0 =A0 =A0NODEV); >> =A0 =A0 =A0 =A0return (0); >> =A0} Please don't put binary characters in mail. >> + >> +static int >> +sysctl_vm_swap_free(SYSCTL_HANDLER_ARGS) { First style bug: misplaced brace. >> + =A0 =A0 =A0 int swap_free, used; >> + =A0 =A0 =A0 int total; Second and third style bugs: int variables not altogther, and not sorted. But these are probably actually type and overflow errors (bugs 4 and 5)... >> + >> + =A0 =A0 =A0 swap_pager_status(&total, &used); >> + Bug 6 is a style bug (extra blank line). >> + =A0 =A0 =A0 swap_free =3D (total - used) * PAGE_SIZE; We multiply by PAGE_SIZE. This can probably overflow at 2G. Then assigning to the int variable overflows at the same point. This gives bugs 4 and 5. Related sysctls for memory sizes (see kern_mib.c) avoid this problem by using u_long instead of int. But for disk sizes, using u_long only reduces the problem to overflow at 4G, since systems with 32-bit longs can have larger disks than memory, and large disks can have large swap. I'm not sure if old restrictions on swap size have been fixed so that more than 2G can actually be allocated, but most places that muliply by PAGE_SIZE aee now careful to use expressions like '(vm_ooffset_t)nblks * PAGE_SIZE' and to assign the result to a variable of type vm_ooffset_t. swap_total is one such variable. But its sysctl has type errors too. vm_ooffset_t is just not a supported type in sysctl. SYSCTL_QUAD() is used for it. Quads shouldn't exist, and SYSCTL_QUAD() should never be used, especially for non-quads. There are now some support for 64-bit types in sysctl. Using these would be less bogus. Bug 7 is a style bug: the related sysctls for memory sizes use ctob() instead of hard-coding PAGE_SIZE. Avoiding this style bug also avoids multiplication overflow (else you need a cast to go above 2G starting with an int page count). ctob() is bogus too (seen any clicks lately?). >> + =A0 =A0 =A0 return SYSCTL_OUT(req, &swap_free, sizeof(swap_free)); Bug 8 is a style bug (no spaces around return value). >> +} >> + >> +SYSCTL_OID(_vm, OID_AUTO, swap_free, CTLTYPE_INT|CTLFLAG_RD|CTLFLAG_MPS= AFE, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 NULL, 0, sysctl_vm_swap_free, "Q", >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 "Blocks of free swap storage."); Bug 9 is a style bug. I didn't even know that the raw SYSCTL_OID() could be misused like this. The normal SYSCTL_PROC() is identical with SYSCTL_OID() except it checks that the access flags are not 0. Few or no SYSCTL_FOO()s have no access flags, and this is not one. It has rather excessive access flags (I think CTLFLAG_MPSAFE is unnecessary. It is not used for the related memory sysctls). vm has 4 existing SYSCTL_OID()s; kern has 3; ia64/ia64 has 1; i386/i386 has 1; netipsec has 1. These are the only matches for ^SYSCTL_OID in /sys, and they all seem to be just style bugs. Bug 10 is a collection of style bugs (missing spaces around binary operator '|'). Bug 11 is a collection of style bugs (weird 2-tab continuation indentation instead of the normal 4 spaces. 5 out of 7 existing SYSCTL_*()s in this file including the vm_swap ones use normal continuation indentation. Bug 12 is the most serious type error. The format is "Q", but only an int is returned. I don't see how this can result in anything except garbage printing in sysctl(8). The access flag gives the type correctly as int, but sysctl(8) mostly uses the format string for output. Oops, that was in an old version. sysctl(8) now mostly uses the access flag, and has no literal Q's in it any more. So this error might not be serious, depending on whether the bad format string is actually used. The sysctl data doesn't give the size of type type, but leaves it as 0. This works because the size is given as sizeof(swap_free) in the call to SYSCTL_OUT(). This can be confusing, and use of the raw SYSCTL_OUT() should be avoided if possible. Many SYSCTL_PROC() routines and of course all macros like SYSCTL_INT() use sysctl_handle_foo() to handle integers. For this, the size is unfortunately encoded in the name of the function (so the function must be matched to the actual type in callers) and and is hard-coded in sysctl_handle_foo(). But unlike the raw SYSCTL_OUT(), sysctl_handle_foo() can in theory check for consistency between its name and the types in the data and thus detect type errors like the "Q" in the above. Bug 13 is a style bug (termination of the sysctl description with a "."). Bruce --0-1047696930-1338542049=:1865--