Date: Fri, 1 Jun 2012 19:14:09 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Sergey Kandaurov <pluknet@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler <eadler@FreeBSD.org> Subject: Re: svn commit: r236380 - head/sys/vm Message-ID: <20120601175403.H1865@besplex.bde.org> In-Reply-To: <CAE-mSOK=qyKbTwnKx_y5VmDNdYJG_K7R4j6565hWy09gEu_wZQ@mail.gmail.com> References: <201206010442.q514gqqv084148@svn.freebsd.org> <CAE-mSOK=qyKbTwnKx_y5VmDNdYJG_K7R4j6565hWy09gEu_wZQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <eadler@freebsd.org> wrote:
>> ...
>> Log:
>> =A0Add sysctl to query amount of swap space free
>>
>> =A0PR: =A0 =A0 =A0 =A0 =A0 kern/166780
>> =A0Submitted by: Radim Kolar <hsn@sendmail.cz>
>> =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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120601175403.H1865>