Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[Conrad Meyer]

On Thu, Oct 22, 2015 at 6:10 PM, NGie Cooper  wrote:
> On Thu, Oct 22, 2015 at 4:03 PM, Conrad E. Meyer  wrote:
>> URL: https://svnweb.freebsd.org/changeset/base/289773
>>
>> Log:
>>   Sysctl: Add common support for U8, U16 types
>
> - This is missing documentation in sysctl(9).

r289831, thanks.

> - It probably deserves `Relnotes: yes`.

I don't think so.

> - Shouldn't __FreeBSD_version be bumped for this change?

What is __FreeBSD_version and why would it be bumped?

Best,
Conrad
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[NGie Cooper]

On Fri, Oct 23, 2015 at 2:06 PM, Mark Linimon  wrote:
> On Fri, Oct 23, 2015 at 08:09:35AM -0700, Conrad Meyer wrote:
>> What is __FreeBSD_version and why would it be bumped?
>
> __FreeBSD_version was introduced in the following commit:
>
>   https://svnweb.freebsd.org/base/head/sys/sys/param.h?r1=34924=36260
>
> where it has been ever since:
>
>   https://svnweb.freebsd.org/base/head/sys/sys/param.h?view=log
>
> This variable exists to tell the Ports Collection, among others,
> that "something has changed that may require you to patch and/or
> recompile."
>
> The historical values are documented in
>
>   
> https://www.freebsd.org/doc/en/books/porters-handbook/versions.html#freebsd-versions-table
>
> Your mentor should have shown you this information.

It's also really handy if you know that a KPI exists after `#if
__FreeBSD_version > XYZ`. It doesn't work for folks who cherry-pick
(like we do at $work, iXsystems with "NextBSD"/"TrueOS", etc), but it
works for some upstream folks -- especially when you have something
that uses a KPI in flux on a CURRENT branch for instance... *cough*
see open-vm-tools and breakage due to vm(4) APIs being in flux (
:/..).
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[Mark Linimon]

On Fri, Oct 23, 2015 at 08:09:35AM -0700, Conrad Meyer wrote:
> What is __FreeBSD_version and why would it be bumped?

__FreeBSD_version was introduced in the following commit:

  https://svnweb.freebsd.org/base/head/sys/sys/param.h?r1=34924=36260

where it has been ever since:

  https://svnweb.freebsd.org/base/head/sys/sys/param.h?view=log

This variable exists to tell the Ports Collection, among others,
that "something has changed that may require you to patch and/or
recompile."

The historical values are documented in

  
https://www.freebsd.org/doc/en/books/porters-handbook/versions.html#freebsd-versions-table

Your mentor should have shown you this information.

mcl
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[Conrad Meyer]

On Fri, Oct 23, 2015 at 2:06 PM, Mark Linimon  wrote:
> On Fri, Oct 23, 2015 at 08:09:35AM -0700, Conrad Meyer wrote:
>> What is __FreeBSD_version and why would it be bumped?
>
> 
>
> This variable exists to tell the Ports Collection, among others,
> that "something has changed that may require you to patch and/or
> recompile."

Ok.  Nothing has changed that may require ports to patch and/or
recompile, so I don't think this needs to be bumped.

Best,
Conrad
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[Ian Lepore]

On Fri, 2015-10-23 at 14:43 -0700, Conrad Meyer wrote:
> On Fri, Oct 23, 2015 at 2:06 PM, Mark Linimon 
> wrote:
> > On Fri, Oct 23, 2015 at 08:09:35AM -0700, Conrad Meyer wrote:
> > > What is __FreeBSD_version and why would it be bumped?
> > 
> > 
> > 
> > This variable exists to tell the Ports Collection, among others,
> > that "something has changed that may require you to patch and/or
> > recompile."
> 
> Ok.  Nothing has changed that may require ports to patch and/or
> recompile, so I don't think this needs to be bumped.
> 
> Best,
> Conrad
> 

"ports need recompile" is only one of several reasons to bump the
version.  Another is making it possible to test for new features that
arrived with a given version, and that's what you've done.

Suppose I maintain an out-of-tree driver that has to build on several
versions, and uint16 is really the right type for its sysctl but it
also has to work on versions that don't have that support.  That tends
to get handled with things like #if __FreeBSD_version < . 
 (Contrived example here maybe, since if uint32 worked on one version,
I'd likely use it on all of versions.)

One of the implications of the feature-availability testing is that
when you MFC your change, you also have to bump the version number on
that branch (independently, not via MFC).

-- Ian
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[Conrad Meyer]

On Fri, Oct 23, 2015 at 2:56 PM, Ian Lepore  wrote:
> On Fri, 2015-10-23 at 14:43 -0700, Conrad Meyer wrote:
>> On Fri, Oct 23, 2015 at 2:06 PM, Mark Linimon 
>> wrote:
>> > On Fri, Oct 23, 2015 at 08:09:35AM -0700, Conrad Meyer wrote:
>> > > What is __FreeBSD_version and why would it be bumped?
>> >
>> > 
>> >
>> > This variable exists to tell the Ports Collection, among others,
>> > that "something has changed that may require you to patch and/or
>> > recompile."
>
> Another is making it possible to test for new features that
> arrived with a given version, and that's what you've done.
>
> Suppose I maintain an out-of-tree driver that has to build on several
> versions, and uint16 is really the right type for its sysctl but it
> also has to work on versions that don't have that support.  That tends
> to get handled with things like #if __FreeBSD_version < .
>  (Contrived example here maybe, since if uint32 worked on one version,
> I'd likely use it on all of versions.)

Suppose you did. The change lends itself to "#ifdef CTLTYPE_U16" — in
this truly contrived scenario. Any other check is redundant or less
specific. (As Ngie points out, checking an absolute version doesn't do
you a lot of good in the face of backports/cherry-picks.)

> One of the implications of the feature-availability testing is that
> when you MFC your change, you also have to bump the version number on
> that branch (independently, not via MFC).

Not a problem — the MFC After field was intentionally omitted (i.e.,
"MFC After: never"). This is only intended for CURRENT.

Best,
Conrad
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[NGie Cooper]

On Thu, Oct 22, 2015 at 4:03 PM, Conrad E. Meyer  wrote:
> Author: cem
> Date: Thu Oct 22 23:03:06 2015
> New Revision: 289773
> URL: https://svnweb.freebsd.org/changeset/base/289773
>
> Log:
>   Sysctl: Add common support for U8, U16 types
>
>   Sponsored by: EMC / Isilon Storage Division

- This is missing documentation in sysctl(9).
- It probably deserves `Relnotes: yes`.
- Shouldn't __FreeBSD_version be bumped for this change?

Thanks!
-NGie
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[Conrad E. Meyer]

Author: cem
Date: Thu Oct 22 23:03:06 2015
New Revision: 289773
URL: https://svnweb.freebsd.org/changeset/base/289773

Log:
  Sysctl: Add common support for U8, U16 types
  
  Sponsored by: EMC / Isilon Storage Division

Modified:
  head/sbin/sysctl/sysctl.c
  head/sys/kern/kern_sysctl.c
  head/sys/sys/sysctl.h

Modified: head/sbin/sysctl/sysctl.c
==
--- head/sbin/sysctl/sysctl.c   Thu Oct 22 22:29:25 2015(r289772)
+++ head/sbin/sysctl/sysctl.c   Thu Oct 22 23:03:06 2015(r289773)
@@ -96,6 +96,8 @@ static int ctl_size[CTLTYPE+1] = {
[CTLTYPE_ULONG] = sizeof(u_long),
[CTLTYPE_S64] = sizeof(int64_t),
[CTLTYPE_U64] = sizeof(uint64_t),
+   [CTLTYPE_U8] = sizeof(uint8_t),
+   [CTLTYPE_U16] = sizeof(uint16_t),
 };
 
 static const char *ctl_typename[CTLTYPE+1] = {
@@ -105,6 +107,8 @@ static const char *ctl_typename[CTLTYPE+
[CTLTYPE_ULONG] = "unsigned long",
[CTLTYPE_S64] = "int64_t",
[CTLTYPE_U64] = "uint64_t",
+   [CTLTYPE_U8] = "uint8_t",
+   [CTLTYPE_U16] = "uint16_t",
 };
 
 static void
@@ -221,6 +225,8 @@ parse(const char *string, int lineno)
int len, i, j;
const void *newval;
const char *newvalstr = NULL;
+   uint8_t u8val;
+   uint16_t u16val;
int intval;
unsigned int uintval;
long longval;
@@ -322,6 +328,8 @@ parse(const char *string, int lineno)
}
 
switch (kind & CTLTYPE) {
+   case CTLTYPE_U8:
+   case CTLTYPE_U16:
case CTLTYPE_INT:
case CTLTYPE_UINT:
case CTLTYPE_LONG:
@@ -345,6 +353,16 @@ parse(const char *string, int lineno)
errno = 0;
 
switch (kind & CTLTYPE) {
+   case CTLTYPE_U8:
+   u8val = (uint8_t)strtoul(newvalstr, , 0);
+   newval = 
+   newsize = sizeof(u8val);
+   break;
+   case CTLTYPE_U16:
+   u16val = (uint16_t)strtoul(newvalstr, , 
0);
+   newval = 
+   newsize = sizeof(u16val);
+   break;
case CTLTYPE_INT:
if (strncmp(fmt, "IK", 2) == 0)
intval = strIKtoi(newvalstr, , 
fmt);
@@ -890,6 +908,8 @@ show_var(int *oid, int nlen)
free(oval);
return (0);
 
+   case CTLTYPE_U8:
+   case CTLTYPE_U16:
case CTLTYPE_INT:
case CTLTYPE_UINT:
case CTLTYPE_LONG:
@@ -902,6 +922,14 @@ show_var(int *oid, int nlen)
sep1 = "";
while (len >= intlen) {
switch (kind & CTLTYPE) {
+   case CTLTYPE_U8:
+   umv = *(uint8_t *)p;
+   mv = *(int8_t *)p;
+   break;
+   case CTLTYPE_U16:
+   umv = *(uint16_t *)p;
+   mv = *(int16_t *)p;
+   break;
case CTLTYPE_INT:
case CTLTYPE_UINT:
umv = *(u_int *)p;

Modified: head/sys/kern/kern_sysctl.c
==
--- head/sys/kern/kern_sysctl.c Thu Oct 22 22:29:25 2015(r289772)
+++ head/sys/kern/kern_sysctl.c Thu Oct 22 23:03:06 2015(r289773)
@@ -1128,6 +1128,70 @@ static SYSCTL_NODE(_sysctl, 5, oiddescr,
  */
 
 /*
+ * Handle an int8_t, signed or unsigned.
+ * Two cases:
+ * a variable:  point arg1 at it.
+ * a constant:  pass it in arg2.
+ */
+
+int
+sysctl_handle_8(SYSCTL_HANDLER_ARGS)
+{
+   int8_t tmpout;
+   int error = 0;
+
+   /*
+* Attempt to get a coherent snapshot by making a copy of the data.
+*/
+   if (arg1)
+   tmpout = *(int8_t *)arg1;
+   else
+   tmpout = arg2;
+   error = SYSCTL_OUT(req, , sizeof(tmpout));
+
+   if (error || !req->newptr)
+   return (error);
+
+   if (!arg1)
+   error = EPERM;
+   else
+   error = SYSCTL_IN(req, arg1, sizeof(tmpout));
+   return (error);
+}
+
+/*
+ * Handle an int16_t, signed or unsigned.
+ * Two cases:
+ * a variable:  point arg1 at it.
+ * a constant:  pass it in arg2.
+ */
+
+int
+sysctl_handle_16(SYSCTL_HANDLER_ARGS)
+{
+   int16_t tmpout;
+   int error = 0;
+
+   /*
+* Attempt to get a coherent snapshot by making a copy of the data.
+*/
+   if (arg1)
+   tmpout = *(int16_t *)arg1;
+   else
+   tmpout = arg2;
+   error = SYSCTL_OUT(req, , 

Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys

[Bruce Evans]

On Thu, 22 Oct 2015, Conrad E. Meyer wrote:


Log:
 Sysctl: Add common support for U8, U16 types

 Sponsored by:  EMC / Isilon Storage Division


This uses similar low-quality code to other sysctls, and more.


Modified: head/sbin/sysctl/sysctl.c
==
--- head/sbin/sysctl/sysctl.c   Thu Oct 22 22:29:25 2015(r289772)
+++ head/sbin/sysctl/sysctl.c   Thu Oct 22 23:03:06 2015(r289773)
@@ -96,6 +96,8 @@ static int ctl_size[CTLTYPE+1] = {
[CTLTYPE_ULONG] = sizeof(u_long),
[CTLTYPE_S64] = sizeof(int64_t),
[CTLTYPE_U64] = sizeof(uint64_t),
+   [CTLTYPE_U8] = sizeof(uint8_t),
+   [CTLTYPE_U16] = sizeof(uint16_t),
};


Unsorting.  The table was sorted on size, then alphabetically.
Alphabetical ordering also happens to give ordering on rank.  (This
ordering is the inverse of the ordering for declarations.)



static const char *ctl_typename[CTLTYPE+1] = {
@@ -105,6 +107,8 @@ static const char *ctl_typename[CTLTYPE+
[CTLTYPE_ULONG] = "unsigned long",
[CTLTYPE_S64] = "int64_t",
[CTLTYPE_U64] = "uint64_t",
+   [CTLTYPE_U8] = "uint8_t",
+   [CTLTYPE_U16] = "uint16_t",
};


Consistent unsorting.



static void
@@ -221,6 +225,8 @@ parse(const char *string, int lineno)
int len, i, j;
const void *newval;
const char *newvalstr = NULL;
+   uint8_t u8val;
+   uint16_t u16val;
int intval;
unsigned int uintval;
long longval;


Locally consistent, but backwards sorting.  The *val variables were sorted
on rank, and the addition matches this, but style(9) requires inverse
sorting on rank.  Other local variables are randomly ordered.


@@ -322,6 +328,8 @@ parse(const char *string, int lineno)
}

switch (kind & CTLTYPE) {
+   case CTLTYPE_U8:
+   case CTLTYPE_U16:
case CTLTYPE_INT:
case CTLTYPE_UINT:
case CTLTYPE_LONG:


Consistent sorting.  The types are sorted on rank.  Style(9) requires
sorting case stylements alpahbetically, but it is good to break that
here and sort on rank or inverse rank to match other orderings.


@@ -345,6 +353,16 @@ parse(const char *string, int lineno)
errno = 0;

switch (kind & CTLTYPE) {
+   case CTLTYPE_U8:
+   u8val = (uint8_t)strtoul(newvalstr, , 0);
+   newval = 
+   newsize = sizeof(u8val);
+   break;


Bug: blind truncation gives overflow before overflow can be checked for.
The overflow checking done by the subsequent errno check only checks for
representability of the return value.

Style bug: bogus cast to make the overflow more explicit and/or prevent
the compiler from detecting the bug and misleading the reader into thinking
that overflow is intentionally allowed.  The cast usually has no other
effect (the behaviour on conversion is only implementation-defined in
the overflow case.  Perhaps it can be different with the cast).

The other assignments are mostly similarly broken:
- CTLTYPE_INT: similar bogus cast to int
- CTLTYPE_UINT: unsimilar bogus cast to int.  The correct cast to u_int
  would have no effect, but casting to int gratuitously gives
  implementation defined behaviour even for representable values like
  UINT_MAX
- CTLTYPE_LONG, CTLTYPE_ULONG: correct (null conversion with no cast)
- CTLTYPE_S64: no cast, though overflow can occur if int64_t < intmax_t
- CTLTYPE_U64: no cast.  Correct since the type is unsigned
- CTLTYPE_IMAX, CTLTYPE_UIMAX: unsupported.


+   case CTLTYPE_U16:
+   u16val = (uint16_t)strtoul(newvalstr, , 
0);
+   newval = 
+   newsize = sizeof(u16val);
+   break;


As a bug, but another style bug from the cast (ling too long).


case CTLTYPE_INT:
if (strncmp(fmt, "IK", 2) == 0)
intval = strIKtoi(newvalstr, , 
fmt);


The error handling is somewhat obfuscated by having general code after the
switch statement to avoid almost repeating it for each case.  It could be
simplified by always assigning to uintmax_t uimaxval.  Then do general
error handling.  Then do specific error handling (range checking for each
case).

The error handling also has a silly check for empty values before the
main check.  The general error handling finds empty values and much
more as a special case of disallowing values with no digits (e.g.,
nonempty leading whitespace and/or sign before the empty value).  So
all the silly check does is give a special error message for empty
values.  Not worth it.

Style bug: you forgot to add the new types to the silly check.


free(oval);