svn commit: r217369 - in head/sys: cam/scsi sys

[mdf at FreeBSD.org]

On Thu, Jan 13, 2011 at 9:56 PM, Bruce Evans <brde at optusnet.com.au> wrote:
> On Thu, 13 Jan 2011, Matthew D Fleming wrote:
>
>> Log:
>>  Add a 64-bit hex-printed sysctl(9) since there is at least one place in
>>  the code that wanted it.  It is named X64 rather than XQUAD since the
>>  quad name is a historical abomination that should not be perpetuated.
>
> :-).  It is only long long that is abominable.  Both are historical.
>
> I think these X formats shouldn't exist, even as defaults.  Instead,
> sysctl(8) should have option(s) to control the format.  I'm used to
> typing lots of "p/x"s to get hex formatting in gdb.  There 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.  The 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.  This could
> be "fixed" by mis-specifying the format with a U, or vice versa.  Since
> 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
>>
>> ==============================================================================
>> --- head/sys/cam/scsi/scsi_da.c Thu Jan 13 18:20:27 2011        (r217368)
>> +++ head/sys/cam/scsi/scsi_da.c Thu Jan 13 18:20:33 2011        (r217369)
>> @@ -1127,9 +1127,9 @@ dasysctlinit(void *context, int pending)
>>                struct ccb_trans_settings_fc *fc = &cts.xport_specific.fc;
>>                if (fc->valid & CTS_FC_VALID_WWPN) {
>>                        softc->wwpn = fc->wwpn;
>> -                       SYSCTL_ADD_XLONG(&softc->sysctl_ctx,
>> +                       SYSCTL_ADD_X64(&softc->sysctl_ctx,
>>                            SYSCTL_CHILDREN(softc->sysctl_tree),
>> -                           OID_AUTO, "wwpn", CTLTYPE_QUAD | CTLFLAG_RD,
>> +                           OID_AUTO, "wwpn", CTLFLAG_RD,
>>                            &softc->wwpn, "World Wide Port Name");
>>                }
>>        }
>>
>
> Hmm, forcing hex might be best for flags (but I'll ask for binary then :-)
> and for mac addresses, but not for inet4 addresses.  I don't know what sort
> of address this is.
>
> Bruce
>