[Linux-ha-dev] Bug in lrmd / OCF RAs

[Alan Robertson]

Sun Jiang Dong wrote:
> Alan Robertson wrote:
> 
>> Sun Jiang Dong wrote:
>>
>>> Alan Robertson wrote:
>>
>>
>>
>>>>
>>>> This *can* be made to work, but not with the code you have in CVS...
>>>>
>>>>  From raexechb.c, line 267:
>>>>         if (params_ht) {
>>>>                 g_hash_table_foreach(params_ht, params_hash_to_argv,
>>>>                                         params_argv);
>>>>         }
>>>>
>>>> This code is wrong.  It will NOT put them in integer order.  It will 
>>>> put 
>>>
>>>
>>>
>>> It works. Please read the detailed code in function params_hash_to_argv
>>> (called in prepare_cmd_parameters) and take the parameter format (in 
>>> hashtable) into account.
>>> ( key="1", value="ipaddr=192.168.1.1")
>>>
>>> BTW, you may reply a obsolete mail ;-), there are some new mails of 
>>> the same subject.
>>
>>
>>
>> AFAIK this was your newest mail I could find on this subject, except 
>> for a whole bunch of later emails which said "see my previous email 
>> where I explained it".
>>
>> You will note that of two people we know who read it besides the 
>> author, that none of them read it and understood it.  This is a clue 
>> that it is unclear.
>>
>> It will only work if nothing but these 1=foo environment variables are 
>> passed to you, and if there are no gaps in the indexes in the CIB.
>>
> I evr supposed the key denoting order is made by porgram, not input 
> directly from CIB (for example, lrmadmin apply the ordering information 
> to it according to command line parameter's position, so the key will ok 
> as expected), so not check it here strictly. Anyway, error and bounds 
> checking need be strengthened, will check my code again, especially in 
> the aspect.

There is NO WAY that anyone else can check this except for a GUI, because 
these constraints are only known by the GUI and your code.  Who else could 
possibly know about this pecularity of the way input has to be prepared 
which is unique to only this type of resource agent?

The CIB will always be editable by humans.

One of the items in the coding standards says "always assume your caller is 
an idiot".  Since the caller preparing your data in this case is a human 
being, then this assumption is true ten times over ;-)

The code currently in CVS is obscure.  Putting bounds checking on it won't 
make it any less obscure.

If you look at the code I suggested in my earlier mail, it doesn't have 
these bounds checking issues, it is simple and easy to understand, and it 
isn't any longer than the code you currently have.  Because it is simple 
and obvious, by design it cannot fail in as many ways as the code you 
currently have.

-- 
     Alan Robertson <alanr at unix.sh>

"Openness is the foundation and preservative of friendship...  Let me claim 
from you at all times your undisguised opinions." - William Wilberforce