Print

---

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