Dear Xrootd Developers,
I have been reading the source of xrootd's load balancer to try to
understand what it does and found some lint and a bug. The line
numbers are approximate.
src/XrdOlb/XrdOlbConfig.cc:
295: NoGo = ConfigProc();
This line should be NoGo |= ConfigProc(); (+= would work too).
Otherwise it trumps the assignment to NoGo a few lines before. It
isn't clear that you even need to bother with the ConfigProc call if
NoGo is already set.
908: xallow - it appears that if the host/netgroup argument is omitted
it defaults to host. So in the config file
allow xyz.stanford.edu
is equivalent to
allow host xyz.standford.edu
If that is correct, the comment at the start of xallow should point
that out. The syntax description does indicate that the host/netgroup
argument is optional, but doesn't state the default. Another solution
is to make it mandatory.
1384: there is a comment about xperf calling a program to write values
into "shared memory". I believe this is out of date, and if so, it is
misleading. If I read the code correctly, the Meter code just execs the
monitor process using custom code that looks a lot like a
reimplementation of popen().
src/XrdOlb/XrdOlbManager.cc
The Snooze method declares maxerrs=3 and later, outside of any loop,
has "if (!maxerrs--) return 0;" which will never be satisfied, since it
will always be 3.
The Add_Manager method (line 984?): The comment before it says
AddMaster instead of Add_Manager and the epname omits the underscore in
the method name.
src/XrdOlb/XrdOlbMeter.cc
This #includes <sys/shm.h>. I suspect this is no longer needed. Other
include files may also be redundant.
src/XrdOuc/XrdOucPthread.cc
At first glance it appears that the code in XrdOucThread_Start() is
equivalent to
return pthread_create(tid, NULL, proc, arg);
If that is correct, it will be easier to read if you reduce it to that.
Likewise, the following method XrdOucThread_Run() would be somewhat
clearer as
int rc;
if ((rc = XrdOucThread_Start(tid, proc, arg)) == 0) { rc =
XrdOucThread_Detach(*tid); }
return rc;
utils/fs_stat: I don't know if this script is used for anything
important, but it goes to much trouble to find the path to the df
program. It then ignores that knowledge and hardwires /bin/df into the
command to get the file system status.
General observations:
1. The configuration file-reading code is effectively replicated 4
times. Presumably there was a time when there were four separate config
files. It might be better to unify it (since there is now effectively
only one config file anyway) and make it table-driven for each
subcomponent so that the code is smaller and more maintainable.
2. There are several unnecessary casts in the code. Casting variables
of type "char *" to (const char *) when passing them as arguments to
functions expecting a (const char *) is redundant. (E.g., calls to
access() frequently have a redundant cast.) I don't find that redundant
casts improve the readability of the code. I stop to ask why it's there
since it appears to be unnecessary and wonder what I am missing, but
others may have a different view of this.
I hope these are helpful.
--
Gregory J. Sharp email: [log in to unmask]
Wilson Synchrotron Laboratory url:
http://www.lepp.cornell.edu/~gregor
Dryden Rd ph: +1 607 255 4882
Ithaca, NY 14853 fax: +1 607 255 8062
|