svn commit: r294246 - user/ngie/socket-tests/tools/regression/sockets/unix_cmsg

[NGie Cooper]

> On Jan 17, 2016, at 21:13, Bruce Evans <brde at optusnet.com.au> wrote:
> 
> On Mon, 18 Jan 2016, Garrett Cooper wrote:
> 
>> Log:
>> Checkpoint work to bump WARNS to 6
>> 
>> Try to fix -Wcast-align issues by using bcopy to assign the values in
>> the structure.
> 
> Using the standard memmove() instead of the BSD bcopy() might be considered
> a style bug.  However, this is a case where memmove() works better.  The
> compiler normally replaces it by __builtin_mmemove() which can be optimized
> better for small fixed-size copies.  On arches without strict alignment
> requirements, this normally results in the same loads and stores that the
> pointer hack would give.
> 
> However2, the kernel build environment is not normal, and changes
> related to this give a mess of style bugs and negative optimizations
> instead of the indended positive ones.  Long ago, the style was broken
> by changing some bcopy()s (mainly in networking code) to memcpy()s and
> adding a fallback extern memcpy() for compilers that don't do the
> optimization (mainly for the -O0 case).  Before that, it was easier
> to enforce the style by not supporting memcpy().  This was turned into
> less than a style bug by adding -ffreestanding to CFLAGS (mainly to
> fix the compiler replacing printf() by __builtin_printf() and turning
> that into puts() in some cases).  -ffreestanding is technically correct,
> but it disables _all_ builtins.  This made the changes to use
> __builtin_memcpy() less than style bugs since the fallback memcpy()
> didn't attempt to be optimal.  However, it is more optimal than bcopy()
> for small copies on some arches.  No one cares about the lost builtins
> since the optimizations that they give are tiny.
> 
> Later, many more mem*() functions were added, mainly to support contribed
> software that doesn't follow BSD style.  The result is a mess of different
> styles in non-contrib'ed code too.

I tried memmove and unfortunately it failed similar to bcopy. I think I need to do “deeper copying” in the structures to ensure that all of the fields are properly copied over in the bcopy, but I’ll look at what gdb says first.

>> ==============================================================================
>> --- user/ngie/socket-tests/tools/regression/sockets/unix_cmsg/unix_cmsg.c	Mon Jan 18 04:24:43 2016	(r294245)
>> +++ user/ngie/socket-tests/tools/regression/sockets/unix_cmsg/unix_cmsg.c	Mon Jan 18 04:32:19 2016	(r294246)
>> @@ -1027,60 +1027,60 @@ check_xucred(const struct xucred *xucred
>> static int
>> check_scm_creds_cmsgcred(struct cmsghdr *cmsghdr)
>> {
>> -	const struct cmsgcred *cmsgcred;
>> +	struct cmsgcred cmsgcred;
> 
> The changes fix many style bugs (pointer variables not spelled with
> a p) but leave many others (verbose variable names).

Agreed. I’ll fix this in the next commit.

>> @@ -1145,15 +1145,15 @@ check_scm_creds_sockcred(struct cmsghdr
>> static int
>> check_scm_timestamp(struct cmsghdr *cmsghdr)
>> {
>> -	const struct timeval *timeval;
>> +	const struct timeval timeval;
> 
> 'timeval' is a bad enough name for a struct tag.  'timeval' is a worse
> variable name since it is the same as the struct tag.  The normal name
> is 'tv’.

I agree in these cases. Unless it’s standard variable naming convention for C (rc/retval/rv being shorthand for “return_value”, etc), I try to stick with more verbose names to make the meaning of variables unambiguous. I agree though… the variable name `timeval` is not incredibly helpful to the reader...

>> @@ -1161,15 +1161,15 @@ check_scm_timestamp(struct cmsghdr *cmsg
>> static int
>> check_scm_bintime(struct cmsghdr *cmsghdr)
>> {
>> -	const struct bintime *bintime;
>> +	const struct bintime bintime;
> 
> Names related to bintimes are generally worse than for timevals, starting
> in the implementation not having prefixes in the struct member names.
> 
> Timespecs are in between.  They have prefixes in the struct member names,
> but POSIX broke these from ts_ to tv_.

Thanks for the feedback!
-NGie