svn commit: r205118 - head/sbin/sysctl

[Bruce Evans]

On Sat, 13 Mar 2010, Garrett Cooper wrote:

>> Log:
>>  Free the memory allocated via strdup.

Why bother?
1. Memory is infinite :-).
2. Memory is infinite relative to sysctl's requirements, especially for
    this strdup.  Normally this strdup is never reached.  It is normally
    reached a whole 1 time for sysctl -a (e.g., on ref9-amd64 now),
    since there is a whole 1 sysctl with a timeval format in the configured
    kernel, and not many more (if any more) than 1 in /sys.

>> Modified: head/sbin/sysctl/sysctl.c
>> ==============================================================================
>> --- head/sbin/sysctl/sysctl.c   Sat Mar 13 11:06:47 2010        (r205117)
>> +++ head/sbin/sysctl/sysctl.c   Sat Mar 13 11:08:57 2010        (r205118)
>> @@ -382,6 +382,7 @@ S_timeval(int l2, void *p)
>>                if (*p2 == '\n')
>>                        *p2 = '\0';
>>        fputs(p1, stdout);
>> +       free(p1);
>>        return (0);
>>  }

The \xa0's are ugly.

>    strdup(3) can fail in that section of code, thus the fputs(3)
> could segfault:

Who cares?
1. strdup() can't fail, since memory is infinite :-)
2. ... since memory is infinite relative to sysctl's requirements (see above).
3. Most systems are (mis)configured with MALLOC_OPTIONS=A..., so if strdup()
    fails it doesn't return, and all the careful code that checks
    malloc() and strdup() for failing becomes just an obfuscation.
    MALLOC_OPTIONS=A... seriously breaks programs that are likely to
    run out of memory and handle this, but such programs became rare when
    address spaces exceeded 64K and rarer when they exceeded 640K...; they
    are now infinite :-), so problems are rarely seen.

The strdup() is bogus anyway.  ctime() returns a non-const string so the
the string should be modified directly.  It is just as easy to avoid the
modification.  The function with this has lots more style bugs:

% static int
% S_timeval(int l2, void *p)

Style bug: *p should be const.

% {
% 	struct timeval *tv = (struct timeval*)p;

Style bug: initialization in declaration.

% 	time_t tv_sec;
% 	char *p1, *p2;
% 
% 	if (l2 != sizeof(*tv)) {
% 		warnx("S_timeval %d != %zu", l2, sizeof(*tv));
% 		return (1);
% 	}
% 	printf(hflag ? "{ sec = %'jd, usec = %'ld } " :
% 		"{ sec = %jd, usec = %ld } ",
% 		(intmax_t)tv->tv_sec, tv->tv_usec);

Style bugs: non-KNF indentation of continued lines.

% 	tv_sec = tv->tv_sec;
% 	p1 = strdup(ctime(&tv_sec));

Style bug: strdup() just adds bloat (especially when you add memory
management and error handling for it).

% 	for (p2=p1; *p2 ; p2++)

Style bug: missing spaces around binary operator.

% 		if (*p2 == '\n')
% 			*p2 = '\0';

To avoid the modification, break here and print the (p2 - p1) characters
starting at p1 using "%.*s" format.  This leads to complete de-bloatation:
1: the newline is always at the end, so it can be located using strlen()
2: strlen() is unnecessary to, since the string always has length 25.
    Thus "%.*s", ..., [p2 - p1 OR strlen(p1) -1] reduces to "%.24s".

% 	fputs(p1, stdout);

There is also a style rule against using fputs()...

% 	free(p1);

So the above lines (here they are again, indented a bit, with some error
handling added for not-quite maximal verboseness):

 	% 	p1 = strdup(ctime(&tv_sec));
 	%	if (p1 == NULL)
 	%		err(1, "strdup");
 	% 	for (p2=p1; *p2 ; p2++)
 	% 		if (*p2 == '\n')
 	% 			*p2 = '\0';
 	% 	fputs(p1, stdout);
 	% 	free(p1);

are a verbose way of writing:

 	printf("%.24s", ctime(&tv_sec));

To reduce verboseness a little more, merge this printf() with the preceding
one.

To increase verboseness for both, add error handling for fputs() and printf().

% 	return (0);
% }