Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Fri, Jan 14, 2011 at 10:27 PM, Bruce Evans b...@optusnet.com.au wrote: On Fri, 14 Jan 2011, Garrett Cooper wrote: On Fri, Jan 14, 2011 at 6:42 PM, Bruce Evans b...@optusnet.com.au wrote: On Fri, 14 Jan 2011 m...@freebsd.org wrote: On Thu, Jan 13, 2011 at 10:50 PM, Bruce Evans b...@optusnet.com.au wrote: On Thu, 13 Jan 2011 m...@freebsd.org wrote: There appear to be 330 uses of SYSCTL and QUAD on the same line in CURRENT. This seems reasonable to change them to S64, U64 and X64 so they correctly reflect the size they operate upon. What do y'all think? Now I suggest delaying this until they can be renamed to a type- generic SYSCTL_INT() (would probably need to be spelled differently, SYSCTL_I() say, even if SYSCTL_INT() was changed at the same time). I'm torn on this one. The compiler knows the type (unless, for SYSCTL_INT, NULL/0 is used, but that is also a compile-time check), but to interpret it requires the use of __builtin_foo which is a gcc extension and not part of standard C. Philosophically, while I like this kind of letting the compiler do the work, if you want C++ you know where to find it. Oops. I think sizeof() and issigned() can be used to determine the type well enough in functions and initialized data (do a fuller type check if the compiler supports it), but I don't know how to do this in static sysctl declarations (since sizeof() can't be used in cpp expressions). Why not just create some dumb testcases that can be run at build time to determine that for you? Well, how? You are given SYSCTL_I(var, ...) and have to convert this to what is now in SYSCTL_INT(), using only the type of var, in hundreds or thousands of files. I don't even know how to do this with a test case for each file, short of parsing all the files. Oops, I do know how to translate from sizeof(var) to CTLTYPE_INT or CTLTYPE_UINT. That's just (sizeof(var) == sizeof(int) ? CTLTYPE_INT : ...). The signness is harder (might need gnu typeof(), but not the recent type checking attributes). This won't convert from SYSCTL_I() existing SYSCTL_INT() (the switch on the size would have to be in an ifdef for that, but sizeof() doesn't work in ifdefs), but it works for generating CTLTYPE_* internally SYSCTL_I(). The difficulty is converting from a bare variable `var' to an integer representing the signedness of its type, without using an unportability like typeof(). With typeof(), this is: /* Only works for arithmetic types: */ #define isinteger(var) ((typeof(var))0.1 == 0) #define issigned(var) ((typeof(var))-1 0) ... This is what I meant: $ cat test_warnings.c #include sys/types.h size_t x = (int) -1; int y = 200L; $ gcc -Wconversion -Wstrict-overflow -Wsign-compare -c test_warnings.c test_size_t.c:3: warning: negative integer implicitly converted to unsigned type test_size_t.c:4: warning: overflow in implicit constant conversion $ With the right CFLAGS and a few properly written tests, and a few make rules, you can figure out what's what pretty easily *shrugs*. Thanks, -Garrett ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Sat, 15 Jan 2011, Garrett Cooper wrote: On Fri, Jan 14, 2011 at 10:27 PM, Bruce Evans b...@optusnet.com.au wrote: On Fri, 14 Jan 2011, Garrett Cooper wrote: On Fri, Jan 14, 2011 at 6:42 PM, Bruce Evans b...@optusnet.com.au wrote: ... Oops. ?I think sizeof() and issigned() can be used to determine the type well enough in functions and initialized data (do a fuller type check if the compiler supports it), but I don't know how to do this in static sysctl declarations (since sizeof() can't be used in cpp expressions). ? Why not just create some dumb testcases that can be run at build time to determine that for you? Well, how? ?You are given SYSCTL_I(var, ...) and have to convert this to what is now in SYSCTL_INT(), using only the type of var, in hundreds or thousands of files. ?I don't even know how to do this with a test ... ? ? ? ?/* Only works for arithmetic types: */ ? ? ? ?#define isinteger(var) ?((typeof(var))0.1 == 0) ? ? ? ?#define issigned(var) ? ((typeof(var))-1 0) ? ? ? ?... This is what I meant: $ cat test_warnings.c #include sys/types.h size_t x = (int) -1; int y = 200L; $ gcc -Wconversion -Wstrict-overflow -Wsign-compare -c test_warnings.c test_size_t.c:3: warning: negative integer implicitly converted to unsigned type test_size_t.c:4: warning: overflow in implicit constant conversion $ With the right CFLAGS and a few properly written tests, and a few make rules, you can figure out what's what pretty easily *shrugs*. That's a lot more parsing than seems reasonable. Anyway, we already depend on gnu __typeof() being avaible for the much more central API of pcpu. All pcpu accesses on amd64 and i386 use it, except curthread (curthread has been hacked to use a more direct asm for compile-time efficiency). SYSCTL_I() works even better that I first thought. It automatically gives support for all typedefed integral types. No SYSCTL_FOO_T()s with messy MD ifdefs for matching up foo_t with an integral type are needed. Instead there are even messier MI ifdefs in SYSCTL_I() :-). But not so many. There are hundreds of typedefed types of interest, but only 8 different integer types to map to (8/16/32/64 bits signed and unsigned). The complication that int64_t is plain long on 64 bit arches but long long on 32-bit arches still arises. CTLTYPE_QUAD can be associated with int64_t on all arches, but the format for printing these is arch-dependent. (Note that quad_t's cannot be printed using %qd, and %qd is unusable, since %qd is just an alias for %lld, while quad_t is an alias for int64_t and int64_t is plain long on 64-bit arches so its format is %ld on these arches and %lld on others.) Bruce___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Sat, Jan 15, 2011 at 6:55 AM, Bruce Evans b...@optusnet.com.au wrote: On Sat, 15 Jan 2011, Garrett Cooper wrote: On Fri, Jan 14, 2011 at 10:27 PM, Bruce Evans b...@optusnet.com.au wrote: On Fri, 14 Jan 2011, Garrett Cooper wrote: On Fri, Jan 14, 2011 at 6:42 PM, Bruce Evans b...@optusnet.com.au wrote: ... Oops. I think sizeof() and issigned() can be used to determine the type well enough in functions and initialized data (do a fuller type check if the compiler supports it), but I don't know how to do this in static sysctl declarations (since sizeof() can't be used in cpp expressions). Why not just create some dumb testcases that can be run at build time to determine that for you? Well, how? You are given SYSCTL_I(var, ...) and have to convert this to what is now in SYSCTL_INT(), using only the type of var, in hundreds or thousands of files. I don't even know how to do this with a test ... /* Only works for arithmetic types: */ #define isinteger(var) ((typeof(var))0.1 == 0) #define issigned(var) ((typeof(var))-1 0) ... This is what I meant: $ cat test_warnings.c #include sys/types.h size_t x = (int) -1; int y = 200L; $ gcc -Wconversion -Wstrict-overflow -Wsign-compare -c test_warnings.c test_size_t.c:3: warning: negative integer implicitly converted to unsigned type test_size_t.c:4: warning: overflow in implicit constant conversion $ With the right CFLAGS and a few properly written tests, and a few make rules, you can figure out what's what pretty easily *shrugs*. That's a lot more parsing than seems reasonable. Anyway, we already depend on gnu __typeof() being avaible for the much more central API of pcpu. All pcpu accesses on amd64 and i386 use it, except curthread (curthread has been hacked to use a more direct asm for compile-time efficiency). SYSCTL_I() works even better that I first thought. It automatically gives support for all typedefed integral types. No SYSCTL_FOO_T()s with messy MD ifdefs for matching up foo_t with an integral type are needed. Instead there are even messier MI ifdefs in SYSCTL_I() :-). But not so many. There are hundreds of typedefed types of interest, but only 8 different integer types to map to (8/16/32/64 bits signed and unsigned). The complication that int64_t is plain long on 64 bit arches but long long on 32-bit arches still arises. CTLTYPE_QUAD can be associated with int64_t on all arches, but the format for printing these is arch-dependent. (Note that quad_t's cannot be printed using %qd, and %qd is unusable, since %qd is just an alias for %lld, while quad_t is an alias for int64_t and int64_t is plain long on 64-bit arches so its format is %ld on these arches and %lld on others.) The printing is done entirely in user-space, so it's not too bad. I had figured to upcast everything to [u]intmax_t anyways, and what to cast from would just be [u]int32_t and [u]int64_t depending on reported size. Anything smaller should be copyout(9)'d as a 4 bit quantity. But, there's two factors at play here. The first is sysctl(8) which asks for the size when reading and manages it. The second is sysctl(2) where we have code like SCTL_MASK32 so a long on a 64-bit kernel looks like a long to a 32-bit app. It would be simpler to expose this as an 8-byte quantity on 64-bit kernel, and sysctl(8) will deal with this just fine, but that would break some uses of sysctl(2). However, I *think* it is safe to check the sysctl_req's oldidx/oldlen to see what size the user-space expects and cast a kernel long to int before SYSCTL_OUT. $WORK has gotten busy so I haven't had a chance to work on this much but I'm hopeful that the missus will watch the kids this weekend and allow me to hack. Thanks, matthew ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Sat, 15 Jan 2011 m...@freebsd.org wrote: On Sat, Jan 15, 2011 at 6:55 AM, Bruce Evans b...@optusnet.com.au wrote: SYSCTL_I() works even better that I first thought. ?It automatically gives support for all typedefed integral types. ?No SYSCTL_FOO_T()s ... Grrr, my sentence breaks of 2 spaces are being echoed as 1 space and 1 hard \xa0. The printing is done entirely in user-space, so it's not too bad. I had figured to upcast everything to [u]intmax_t anyways, and what to cast from would just be [u]int32_t and [u]int64_t depending on reported size. Anything smaller should be copyout(9)'d as a 4 bit quantity. I think it shouldn't be converted in the kernel. Applications using sysctl already have to deal with fields shorter than int in structs returned by sysctl(). They don't need to deal with integers shorter than int only since sysctl() doesn't support such integers yet. Short integers could still be returned as essentially themselves by packing them into a struct with nothing else. But, there's two factors at play here. The first is sysctl(8) which asks for the size when reading and manages it. The second is sysctl(2) where we have code like SCTL_MASK32 so a long on a 64-bit kernel looks like a long to a 32-bit app. It would be simpler to expose this as an 8-byte quantity on 64-bit kernel, and sysctl(8) will deal with this just fine, but that would break some uses of sysctl(2). What are the uses? I guess they are not-so-hard-coding data types as int and long, etc., and hard-coding them as int32_t and int64_t. Then you get an error when the kernel returns an unexpected length, but unexpected lengths are expected for 32-bit apps on 64-bit kernels. BTW, short writes are still horribly broken. Suppose there is a size mismatch causing an application tries to write 32 bits to a 64-bit kernel variable. Then no error is detected, and the kernel variable ois left with garbage in its top bits. The error detection last worked with 4.4BSD sysctl, and is still documented to work: % [EINVAL] A non-null newp is given and its specified length in % newlen is too large or too small. I think the case where newlen is too large still works. I guess the bug is potentially larger for 32 bit compat applications, but it is rarely seen for those too since only system management applications should be writing to kernel variables and there is no reason to run such applications in 32 bit mode. The bug affects mainly cases where there is a kernel type mismatch, like using sysctl_handle_int() on a 64-bit variable. I forget how short reads were not detected as errors. It still takes the size in the sysctl data to be for 64 bits for this error to be detectable for either read or write. But your recent changes should fix all such type mismatches. However, I *think* it is safe to check the sysctl_req's oldidx/oldlen to see what size the user-space expects and cast a kernel long to int before SYSCTL_OUT. Maybe if the value fits. The application might be probing for a size that works. Then it shouldn't care what size the value is returned in, provided the value fits. Writing is only slightly more delicate. The application must use a size in which the value fits. Then the kernel can expand the size if necessary. It just has to be careful to fill the top bits and not have sign extension bugs. sysctl_handle_i() can probably handle both directions. Lower-level sysctl code should still return an error if there is a size mismatch. If a value doesn't fit, then the application will just have to detect the error and try a larger size (or fail if it doesn't support larger size). An example might be a 32-bit app reading 64-bit kernel pointers. These probably have the high bits set, so they won't fit in 32-bit sizes. But the application can easily read them using 64-bit sizes. Integers and pointers larger than 64 bits would be more interesting, since a compat application might not have any integer data type larger enough to represent them. So I changed my mind about converting in the kernel. Just try to convert to whatever size the application is trying to use. Don't even force = 32 bit sizes on applications. $WORK has gotten busy so I haven't had a chance to work on this much but I'm hopeful that the missus will watch the kids this weekend and allow me to hack. You seem to have found time :-). Bruce___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Sat, Jan 15, 2011 at 7:06 PM, Bruce Evans b...@optusnet.com.au wrote: On Sat, 15 Jan 2011 m...@freebsd.org wrote: On Sat, Jan 15, 2011 at 6:55 AM, Bruce Evans b...@optusnet.com.au wrote: The printing is done entirely in user-space, so it's not too bad. I had figured to upcast everything to [u]intmax_t anyways, and what to cast from would just be [u]int32_t and [u]int64_t depending on reported size. Anything smaller should be copyout(9)'d as a 4 bit quantity. I think it shouldn't be converted in the kernel. Applications using sysctl already have to deal with fields shorter than int in structs returned by sysctl(). They don't need to deal with integers shorter than int only since sysctl() doesn't support such integers yet. Short integers could still be returned as essentially themselves by packing them into a struct with nothing else. But, there's two factors at play here. The first is sysctl(8) which asks for the size when reading and manages it. The second is sysctl(2) where we have code like SCTL_MASK32 so a long on a 64-bit kernel looks like a long to a 32-bit app. It would be simpler to expose this as an 8-byte quantity on 64-bit kernel, and sysctl(8) will deal with this just fine, but that would break some uses of sysctl(2). What are the uses? I guess they are not-so-hard-coding data types as int and long, etc., and hard-coding them as int32_t and int64_t. Then you get an error when the kernel returns an unexpected length, but unexpected lengths are expected for 32-bit apps on 64-bit kernels. BTW, short writes are still horribly broken. Suppose there is a size mismatch causing an application tries to write 32 bits to a 64-bit kernel variable. Then no error is detected, and the kernel variable ois left with garbage in its top bits. The error detection last worked with 4.4BSD sysctl, and is still documented to work: % [EINVAL] A non-null newp is given and its specified length in % newlen is too large or too small. I think the case where newlen is too large still works. I guess the bug is potentially larger for 32 bit compat applications, but it is rarely seen for those too since only system management applications should be writing to kernel variables and there is no reason to run such applications in 32 bit mode. At Isilon all of userland is still 32-bit only. The reason is that for a while we supported 32-bit and 64-bit kernels in the field, and in fact for various reasons on install the 32-bit kernel always came up first and a reboot after install was forced for 64-bit capable hardware. But we needed the same userspace to work on both kernels. However, I *think* it is safe to check the sysctl_req's oldidx/oldlen to see what size the user-space expects and cast a kernel long to int before SYSCTL_OUT. Maybe if the value fits. The application might be probing for a size that works. Then it shouldn't care what size the value is returned in, provided the value fits. Writing is only slightly more delicate. The application must use a size in which the value fits. Then the kernel can expand the size if necessary. It just has to be careful to fill the top bits and not have sign extension bugs. sysctl_handle_i() can probably handle both directions. Lower-level sysctl code should still return an error if there is a size mismatch. Probing for a size that works should be passing in NULL for the old ptr to get a hint, and for scalars the sie doesn't change. It would be a somewhat malformed app that probes for the size anywhere below e.g. 512 bytes or 1 page. If a value doesn't fit, then the application will just have to detect the error and try a larger size (or fail if it doesn't support larger size). An example might be a 32-bit app reading 64-bit kernel pointers. These probably have the high bits set, so they won't fit in 32-bit sizes. But the application can easily read them using 64-bit sizes. Integers and pointers larger than 64 bits would be more interesting, since a compat application might not have any integer data type larger enough to represent them. So I changed my mind about converting in the kernel. Just try to convert to whatever size the application is trying to use. Don't even force = 32 bit sizes on applications. The proposed code does attempt to convert to whatever the app uses, but it assumes the app is using at least a 32-bit quantity. :-) More on the other thread. Thanks, matthew ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Thu, Jan 13, 2011 at 10:50 PM, Bruce Evans b...@optusnet.com.au wrote: On Thu, 13 Jan 2011 m...@freebsd.org wrote: There appear to be 330 uses of SYSCTL and QUAD on the same line in CURRENT. This seems reasonable to change them to S64, U64 and X64 so they correctly reflect the size they operate upon. What do y'all think? Now I suggest delaying this until they can be renamed to a type- generic SYSCTL_INT() (would probably need to be spelled differently, SYSCTL_I() say, even if SYSCTL_INT() was changed at the same time). I'm torn on this one. The compiler knows the type (unless, for SYSCTL_INT, NULL/0 is used, but that is also a compile-time check), but to interpret it requires the use of __builtin_foo which is a gcc extension and not part of standard C. Philosophically, while I like this kind of letting the compiler do the work, if you want C++ you know where to find it. Thanks, matthew ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Thu, Jan 13, 2011 at 9:56 PM, Bruce Evans b...@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/xs 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
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Fri, 14 Jan 2011 m...@freebsd.org wrote: On Thu, Jan 13, 2011 at 10:50 PM, Bruce Evans b...@optusnet.com.au wrote: On Thu, 13 Jan 2011 m...@freebsd.org wrote: There appear to be 330 uses of SYSCTL and QUAD on the same line in CURRENT. ?This seems reasonable to change them to S64, U64 and X64 so they correctly reflect the size they operate upon. What do y'all think? Now I suggest delaying this until they can be renamed to a type- generic SYSCTL_INT() (would probably need to be spelled differently, SYSCTL_I() say, even if SYSCTL_INT() was changed at the same time). I'm torn on this one. The compiler knows the type (unless, for SYSCTL_INT, NULL/0 is used, but that is also a compile-time check), but to interpret it requires the use of __builtin_foo which is a gcc extension and not part of standard C. Philosophically, while I like this kind of letting the compiler do the work, if you want C++ you know where to find it. Oops. I think sizeof() and issigned() can be used to determine the type well enough in functions and initialized data (do a fuller type check if the compiler supports it), but I don't know how to do this in static sysctl declarations (since sizeof() can't be used in cpp expressions). Bruce___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Fri, Jan 14, 2011 at 6:42 PM, Bruce Evans b...@optusnet.com.au wrote: On Fri, 14 Jan 2011 m...@freebsd.org wrote: On Thu, Jan 13, 2011 at 10:50 PM, Bruce Evans b...@optusnet.com.au wrote: On Thu, 13 Jan 2011 m...@freebsd.org wrote: There appear to be 330 uses of SYSCTL and QUAD on the same line in CURRENT. This seems reasonable to change them to S64, U64 and X64 so they correctly reflect the size they operate upon. What do y'all think? Now I suggest delaying this until they can be renamed to a type- generic SYSCTL_INT() (would probably need to be spelled differently, SYSCTL_I() say, even if SYSCTL_INT() was changed at the same time). I'm torn on this one. The compiler knows the type (unless, for SYSCTL_INT, NULL/0 is used, but that is also a compile-time check), but to interpret it requires the use of __builtin_foo which is a gcc extension and not part of standard C. Philosophically, while I like this kind of letting the compiler do the work, if you want C++ you know where to find it. Oops. I think sizeof() and issigned() can be used to determine the type well enough in functions and initialized data (do a fuller type check if the compiler supports it), but I don't know how to do this in static sysctl declarations (since sizeof() can't be used in cpp expressions). Why not just create some dumb testcases that can be run at build time to determine that for you? -Garrett ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Fri, 14 Jan 2011 m...@freebsd.org wrote: On Thu, Jan 13, 2011 at 9:56 PM, Bruce Evans b...@optusnet.com.au wrote: 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 The format would be ignored for CTLTYPEs that describe an integer, and might be passed in a different way for SYSCTL_OID() (it might be attached to one of the other strings). 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. SYSCTL_PROC() and SYSCTL_OPAQUE() will always have little structure. I think the format string is meaningless and unused for SYSCTL_PROC(). For SYSCTL_OPAQUE(), it is used to encode the type (just by name). Only 5 types seem to be support, and there should be fewer: - T,dev_t: dev_t isn't important any more, and the kernel doesn't even generate any T,dev_t's for sysctl(8) to handle. - S,clockinfo: a historical mistake from 4.4BSD. It would be more useful to print the fields (all 4 of them) in this struct separately. Each sysctl took lots specialized of code in 4.4BSD, so it was easier to pack even non-closely related fields like the ones in clockinfo together. Even if you struct members together, it is convenient to have them separated in a subtree of a node for the struct. FreeBSD now has zillions more clock sysctls but doesn't pack any of the new ones into old or new structs. - S,timeval: might be useful, but it is used a whole once (for kern.boottime). - S,loadavg: from 4.4BSD. Like S,clockinfo, but not so bad. The fields belong together more. They can now be handled better as an array, like kern.cp_time[s] but unlike kern.clockrate. The only difference in sysctl(8) output would be removal of ornations for the struct (braces). The ornations make the output harder to filter, as do separate tokens, but separate tokens are natural for arrays. The field separator for arrays seems to be a space and that is better than a comma. For S_loadavg, the field separator is also a space and there are no field descriptors, but for S_clockinfo the ornations are very hard to parse -- there are commas in the field separators, and namespace=space before each field. There should only be raw load average here, since load averages are presented better elsewhere. - S,vmtotal. A FreeBSD mistake. vmtotals are presented better elsewhere, e.g., in systat -v and vmstat. If not, then it should have been fixed there instead of bloating here. The multi-line ornately-formatted output generated by this messes up grepping for stuff. There are already rather too many sysctls for individual vmtotal and vmmeter fields. 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. It seems to use CTLTYPE* for parsing user values only. Is `kind' fetched for writing (to the kernel) but not for reading? Bruce___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Fri, 14 Jan 2011, Garrett Cooper wrote: On Fri, Jan 14, 2011 at 6:42 PM, Bruce Evans b...@optusnet.com.au wrote: On Fri, 14 Jan 2011 m...@freebsd.org wrote: On Thu, Jan 13, 2011 at 10:50 PM, Bruce Evans b...@optusnet.com.au wrote: On Thu, 13 Jan 2011 m...@freebsd.org wrote: There appear to be 330 uses of SYSCTL and QUAD on the same line in CURRENT. ?This seems reasonable to change them to S64, U64 and X64 so they correctly reflect the size they operate upon. What do y'all think? Now I suggest delaying this until they can be renamed to a type- generic SYSCTL_INT() (would probably need to be spelled differently, SYSCTL_I() say, even if SYSCTL_INT() was changed at the same time). I'm torn on this one. ?The compiler knows the type (unless, for SYSCTL_INT, NULL/0 is used, but that is also a compile-time check), but to interpret it requires the use of __builtin_foo which is a gcc extension and not part of standard C. Philosophically, while I like this kind of letting the compiler do the work, if you want C++ you know where to find it. Oops. ?I think sizeof() and issigned() can be used to determine the type well enough in functions and initialized data (do a fuller type check if the compiler supports it), but I don't know how to do this in static sysctl declarations (since sizeof() can't be used in cpp expressions). Why not just create some dumb testcases that can be run at build time to determine that for you? Well, how? You are given SYSCTL_I(var, ...) and have to convert this to what is now in SYSCTL_INT(), using only the type of var, in hundreds or thousands of files. I don't even know how to do this with a test case for each file, short of parsing all the files. Oops, I do know how to translate from sizeof(var) to CTLTYPE_INT or CTLTYPE_UINT. That's just (sizeof(var) == sizeof(int) ? CTLTYPE_INT : ...). The signness is harder (might need gnu typeof(), but not the recent type checking attributes). This won't convert from SYSCTL_I() existing SYSCTL_INT() (the switch on the size would have to be in an ifdef for that, but sizeof() doesn't work in ifdefs), but it works for generating CTLTYPE_* internally SYSCTL_I(). The difficulty is converting from a bare variable `var' to an integer representing the signedness of its type, without using an unportability like typeof(). With typeof(), this is: /* Only works for arithmetic types: */ #define isinteger(var) ((typeof(var))0.1 == 0) #define issigned(var) ((typeof(var))-1 0) ... Bruce___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
There appear to be 330 uses of SYSCTL and QUAD on the same line in
CURRENT. This seems reasonable to change them to S64, U64 and X64 so
they correctly reflect the size they operate upon.
What do y'all think?
Thanks,
matthew
On Thu, Jan 13, 2011 at 10:20 AM, Matthew D Fleming m...@freebsd.org wrote:
Author: mdf
Date: Thu Jan 13 18:20:33 2011
New Revision: 217369
URL: http://svn.freebsd.org/changeset/base/217369
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.
Modified:
head/sys/cam/scsi/scsi_da.c
head/sys/sys/sysctl.h
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);
}
}
Modified: head/sys/sys/sysctl.h
==
--- head/sys/sys/sysctl.h Thu Jan 13 18:20:27 2011 (r217368)
+++ head/sys/sys/sysctl.h Thu Jan 13 18:20:33 2011 (r217369)
@@ -245,6 +245,8 @@ SYSCTL_ALLOWED_TYPES(ULONG, unsigned lon
SYSCTL_ALLOWED_TYPES(XLONG, unsigned long *a; long *b; );
SYSCTL_ALLOWED_TYPES(INT64, int64_t *a; long long *b; );
SYSCTL_ALLOWED_TYPES(UINT64, uint64_t *a; unsigned long long *b; );
+SYSCTL_ALLOWED_TYPES(XINT64, uint64_t *a; int64_t *b;
+ unsigned long long *c; long long *d; );
#ifdef notyet
#define SYSCTL_ADD_ASSERT_TYPE(type, ptr) \
@@ -389,7 +391,6 @@ SYSCTL_ALLOWED_TYPES(UINT64, uint64_t *a
SYSCTL_ADD_ASSERT_TYPE(INT64, ptr), 0, \
sysctl_handle_quad, Q, __DESCR(descr))
-/* Oid for a quad. The pointer must be non NULL. */
#define SYSCTL_UQUAD(parent, nbr, name, access, ptr, val, descr)
\
SYSCTL_ASSERT_TYPE(UINT64, ptr, parent, name); \
SYSCTL_OID(parent, nbr, name, \
@@ -402,6 +403,18 @@ SYSCTL_ALLOWED_TYPES(UINT64, uint64_t *a
SYSCTL_ADD_ASSERT_TYPE(UINT64, ptr), 0, \
sysctl_handle_quad, QU, __DESCR(descr))
+#define SYSCTL_X64(parent, nbr, name, access, ptr, val, descr) \
+ SYSCTL_ASSERT_TYPE(XINT64, ptr, parent, name); \
+ SYSCTL_OID(parent, nbr, name, \
+ CTLTYPE_QUAD | CTLFLAG_MPSAFE | (access), \
+ ptr, val, sysctl_handle_quad, QX, descr)
+
+#define SYSCTL_ADD_X64(ctx, parent, nbr, name, access, ptr, descr)
\
+ sysctl_add_oid(ctx, parent, nbr, name, \
+ CTLTYPE_QUAD | CTLFLAG_MPSAFE | (access), \
+ SYSCTL_ADD_ASSERT_TYPE(XINT64, ptr), 0, \
+ sysctl_handle_quad, QX, __DESCR(descr))
+
/* Oid for an opaque object. Specified by a pointer and a length. */
#define SYSCTL_OPAQUE(parent, nbr, name, access, ptr, len, fmt, descr) \
SYSCTL_OID(parent, nbr, name, CTLTYPE_OPAQUE|(access), \
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
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/xs 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.
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.
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
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r217369 - in head/sys: cam/scsi sys
On Thu, 13 Jan 2011 m...@freebsd.org wrote: There appear to be 330 uses of SYSCTL and QUAD on the same line in CURRENT. This seems reasonable to change them to S64, U64 and X64 so they correctly reflect the size they operate upon. What do y'all think? Now I suggest delaying this until they can be renamed to a type- generic SYSCTL_INT() (would probably need to be spelled differently, SYSCTL_I() say, even if SYSCTL_INT() was changed at the same time). Bruce ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
