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);
% }_______________________________________________
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"
