On Thu, 2006-07-13 at 22:13 -0400, Daniel Richard G. wrote: > Hello hello, > > First off, is there a public CVS/SVN repository for autofs?
There is a CVS repo but I'm going to stop updating it soon. Use the git repo under http://www.kernel.org/git/ > > I grabbed the -rc1 tarball, figured I'd try building it with my usual devel > CFLAGS, and... wow. This code's got _issues_, maaan.... Nasty young man! So you've forced me to go through the cleanup exercise. Had to be done sooner or later anyway. > > See the attached omnibus patch. Commentary follows, in patch order (more or > less). Of course, not all these changes need to go in as-is, but they > should at least bring attention to some spots that could really use it. > > > (various files): > > * Don't use "#if WITH_FOO", as this produces a warning if WITH_FOO is > undefined. Use "#ifdef". Oops. > > daemon/automount.c: > > * 'mc' is used many times as a local variable in this file; rename the > global instance to 'mrc' to avoid gratuitous shadowing > > * 'mrc' (nee 'mc') is not needed outside of this file; declare as static > > * Two fields in struct master_readmap_cond were not specified in the > initializer for 'mrc' (nee 'mc') > > * 'sc' is used many times as a local variable in this file; rename the > global instance to 'suc' to avoid gratuitous shadowing Yep. Ugly fixed. > > include/automount.h: > > * Disable the OpenBSD syslog.h #include with a cpp conditional, not a > comment. This is a bit safer. Not relevant, that stuffs going, actually nearly gone. > > * The "extern struct master_readmap_cond mc" declaration is not used > > include/lookup_ldap.h: > > * Added appropriate WITH_KERBEROS and WITH_SASL cpp conditionals, to avoid > breakage when the respective libraries are not used. > > (Note: WITH_KERBEROS is a new conditional; support for it needs to be > added to the configure script) Don't think I'll do this. The SASL code is dependent on the Kerberos stuff so I really need to make the SASL conditional on it. But currently there aren't any checks for the Kerberos libs in configure.in so this will have to be done a bit later. > > * Comment the #endifs if the opening #ifdef is far away I don't like to do this. > > include/state.h: > > * Removed a superfluous comma Oops. > > lib/cache.c: > > * Eliminated "implicit cast to unsigned type" warnings > > lib/cat_path.c: > > * Tweaked the _strlen() function: Eliminated the unnecessary de-consting > cast, and rewrote the logic w.r.t. 'max' (this is unsigned, therefore the > max<0 check is pointless; it's not nice to modify your arguments; pointer > arithmetic sucks) > > P.S.: _strlen() should really have a return type of size_t, but I didn't > care enough to change this. Now this was a good catch. Fixed. > > lib/master.c: > > * Eliminated a stray semicolon Oops. > > * Straightened out some unnecessary variable const-ness > > lib/master_parse.y: > > * #define YYENABLE_NLS and YYLTYPE_IS_TRIVIAL to zero; #ifdefs with these > appear in the generated code, and you get warnings if they are not > defined Yep and there were similar issues with the Flex modules. Fixed. > > lib/mounts.c: > > * m_dev and m_ino are 'long unsigned int' values, so cast them to avoid > sscanf() format warnings Fixed. > > lib/nss_parse.y: > > * Same deal with YYENABLE_NLS and YYLTYPE_IS_TRIVIAL Ditto above. > > * Aaaarg! C++ comments in C code! Kill 'em! Kill 'em! Cleaned and sanitized. > > lib/rpc_subs.c: > > * You don't want to use a local variable named 'h_errno'. > > fusion:/tmp$ grep h_errno /usr/include/netdb.h > We use a macro to access always the thread-specific `h_errno' > variable. */ > #define h_errno (*__h_errno_location ()) > ... > > (There were some places that used h_errno without declaring it locally. > I'm assuming that nowhere was this meant to refer to the global netdb.h > h_errno variable.) Double oops, another good catch. Fixed. > > modules/lookup_file.c: > modules/lookup_hosts.c > > * What first caught my attention here was a warning that me->mapent is > being passed in as a format string to sprintf() without further arguments > (lots of fun if there's a "%s" in there somewhere), but that line and the > one before seemed a bit bizarrely written to me. (You call strlen() on a > string, then use sprintf() as an unsafe strcpy() replacement, and grab > the return value to get the length of that same string??) Replaced with > something more straightforward. Hehe, cleaned. > > lookup_ldap.c: > > * What the heck is '?:' supposed to return if the expression is true? (Not > valid C90, at any rate) Ya well, I changed it. > > * My OpenLDAP 2.1.30 header does not define LDAP_SCOPE_ONE. It does, > however, define LDAP_SCOPE_ONELEVEL. OK. caught me on that. Fixed. > > * 'struct mapent me' is unused > > * More mapent/mapent_len silliness And fixed and cleaned. > > modules/mount_nfs.c: > > * This looks like a bug to me. Does it look like a bug to you? Definitely, and found another during the process. Fixed. > > modules/replicated.c: > > * You don't want to use a local variable named h_addr, either. > > fusion:/tmp$ grep h_addr /usr/include/netdb.h > int h_addrtype; /* Host address type. */ > char **h_addr_list; /* List of addresses from name server. > */ > #define h_addr h_addr_list[0] /* Address, for backward compatibility. > */ Aggh .. caught me again, yet another good catch. Fixed. > > > There's still a ton of warnings, mostly of variables shadowing other > variables, but those affect code clarity more so than correctness. > I'm not quite finished the cleanup yet but clone the repo and have another go at it. Ian _______________________________________________ autofs mailing list [email protected] http://linux.kernel.org/mailman/listinfo/autofs
