Date: Fri, 14 Jan 2011 07:22:56 -0800 From: mdf@FreeBSD.org To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r217369 - in head/sys: cam/scsi sys Message-ID: <AANLkTimrxHnupc5kFU8%2BxBi0MC%2BbEiiUf9=aHMG22CM0@mail.gmail.com> In-Reply-To: <20110114162142.M27877@besplex.bde.org> References: <201101131820.p0DIKXip059402@svn.freebsd.org> <20110114162142.M27877@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jan 13, 2011 at 9:56 PM, Bruce Evans <brde@optusnet.com.au> wrote:
> On Thu, 13 Jan 2011, Matthew D Fleming wrote:
>
>> Log:
>> =A0Add a 64-bit hex-printed sysctl(9) since there is at least one place =
in
>> =A0the code that wanted it. =A0It is named X64 rather than XQUAD since t=
he
>> =A0quad name is a historical abomination that should not be perpetuated.
>
> :-). =A0It is only long long that is abominable. =A0Both are historical.
>
> I think these X formats shouldn't exist, even as defaults. =A0Instead,
> sysctl(8) should have option(s) to control the format. =A0I'm used to
> typing lots of "p/x"s to get hex formatting in gdb. =A0There is little
> reason for sysctl(8) to have finer-grained control than gdb (sysctl
> now has hard-coded defaults instead of control).
>
> Now with stricter type checking, even formats for integers are redundant.
> The CTLTYPE now always matches the type, and the format should always
> match the type. =A0The space wasted for the format is 1 pointer plus the
> string.
There are several issues here. First, for OPAQUE types the string is
interpreted by sysctl(8) to know how to interpret the binary blob.
Second, anyone can still use SYSCTL_OID(... CTLTYPE_INT, ..., "QU")
and there's not a lot that can be done to check it. Locally I have
another patch for the various sysctl_handle_foo that asserts the
CTLTYPE_FOO matches the handler, but looking around the FreeBSD code
there's plenty of hand-rolled instances that won't pass; some of these
come from a SYSCTL_PROC setup. Perhaps we could start with a warning
message and upgrade to a KASSERT later.
Thirdly, at the moment sysctl(8) doesn't fetch the access flags and
type specifier and only looks at the string. This is also fixable.
I do agree that the incredibly limited use of XINT, XLONG, and the one
use of X64 mean that it's not a widely used feature, and can probably
be better done with a -x flag to sysctl(8). Most bitflags are still
added as SYSCTL_UINT, so one has to re-interpret the value anyways to
see which bits are set.
> Perhaps there are some remaining type errors involving the kernel type
> being signed when it should be unsigned, or vice versa. =A0This could
> be "fixed" by mis-specifying the format with a U, or vice versa. =A0Since
> the specification is usually done by invoking a SYSCTL*() macro, most
> such fixes, if any, would have been undone by changing the macro to
> match the type, and it would take fixing the kernel type to fix the
> userland format.
The framework is in place to fix the obvious type errors for scalar
types, but I haven't yet finished making the kernel compile cleanly in
universe mode. Until then the check is still if 0'd out.
Thanks,
matthew
>
>> Modified: head/sys/cam/scsi/scsi_da.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/cam/scsi/scsi_da.c Thu Jan 13 18:20:27 2011 =A0 =A0 =A0 =A0=
(r217368)
>> +++ head/sys/cam/scsi/scsi_da.c Thu Jan 13 18:20:33 2011 =A0 =A0 =A0 =A0=
(r217369)
>> @@ -1127,9 +1127,9 @@ dasysctlinit(void *context, int pending)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct ccb_trans_settings_fc *fc =3D &cts=
.xport_specific.fc;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (fc->valid & CTS_FC_VALID_WWPN) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0softc->wwpn =3D fc->wwpn;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 SYSCTL_ADD_XLONG(&softc->s=
ysctl_ctx,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 SYSCTL_ADD_X64(&softc->sys=
ctl_ctx,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0SYSCTL_CHILDREN(s=
oftc->sysctl_tree),
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 OID_AUTO, "wwpn", =
CTLTYPE_QUAD | CTLFLAG_RD,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 OID_AUTO, "wwpn", =
CTLFLAG_RD,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&softc->wwpn, "Wo=
rld Wide Port Name");
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>> =A0 =A0 =A0 =A0}
>>
>
> Hmm, forcing hex might be best for flags (but I'll ask for binary then :-=
)
> and for mac addresses, but not for inet4 addresses. =A0I don't know what =
sort
> of address this is.
>
> Bruce
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimrxHnupc5kFU8%2BxBi0MC%2BbEiiUf9=aHMG22CM0>