Jim, I'm just not a big friend of lengthy and if-in-if unless necessary. Actually, there could be only one if() with all &&, but that seems ugly. You're correct about forgetting to switch logic when || -> &&. Thanks. get_ipmi_uid() -> get_ipmi_user_id() changed, no problem(although it is longer now :]).
v3 attached.
--Duncan
On Thu, Apr 19, 2012 at 11:57 PM, Jim Mankovich <jm...@hp.com> wrote:
> Duncan,
>
> I think get_ipmi_uid(..) could be better written.
>
> I would organize your function so there is only 1 error lprint statement
> instead
> of two since the lprintf statements are exactly the same ( why have 2).
> Also, from
> the previous patch you switched your || to && which I don't think is right
> unless you
> change the logic as well. How about this?
>
> if (arg != NULL && user_id != NULL) {
> if ( (str2uchar(arg, user_id) == 0 &&
> (*user_id >= IPMI_UID_MIN && *user_id <= IPMI_UID_MAX) ) {
> return 0;
> }
> }
> lprintf(LOG_ERR,....)
> return -1;
>
> You might use user_id instead of uid to be more explicit as to the meaning.
>
>
> -- Jim Mankovich | jm...@hp.com --
>
>
> On 4/19/2012 1:45 PM, Duncan Idaho wrote:
>
> Jim,
>
> see attached diff v2.
>
> --Duncan
>
> On Thu, Apr 19, 2012 at 7:49 PM, Jim Mankovich <jm...@hp.com> wrote:
>
>> Duncan,
>>
>> The changes you have made look correct to me and I would agree that using
>> a function or even a
>> #define for the UID verification would be a good idea given there are 4
>> places where the exact
>> same test is done. I also think the error message could be made better
>> by printing the limits being
>> enforced instead of just saying "Invalid user ID: %s"
>>
>> -- Jim Mankovich | jm...@hp.com --
>>
>>
>> On 4/18/2012 11:02 PM, Duncan Idaho wrote:
>>
>> On Wed, Apr 18, 2012 at 8:47 PM, Duncan Idaho <dune.id...@gmail.com>wrote:
>>
>>> Hello all,
>>>
>>> I gave it some thought, and since IPMI specification is quite exact what
>>> range of UIDs is allowed, it seems to be as a good idea to put in more
>>> limitations what could and should be accepted as a "valid" UID. In other
>>> words, what's missing from 'lib/ipmi_user.c' are couple of ``if (uid >
>>> 63)''.
>>> Only con I can think of is a] IPMI implementation that violates IPMI
>>> spec b] a new spec which would allow UIDs > 63, although this seems
>>> unlikely. And since it is noted even in source code itself, I see no
>>> problem to "expand" the limits when such time comes.
>>>
>>> Come to think of it, how about implementation as:
>>> ~~~
>>> #define UID_MIN 1
>>> #define UID_MAX 63
>>>
>>> [...]
>>>
>>> some_function()
>>> {
>>> [...]
>>> if (uid < UID_MIN || uid > UID_MAX) {
>>> /* error */
>>> }
>>> [...]
>>> }
>>> ~~~
>>>
>>> Regards,
>>> Duncan
>>>
>>
>> Please, review attached diff.
>>
>> --Duncan
>>
>>
>>
>> ------------------------------------------------------------------------------
>> For Developers, A Lot Can Happen In A Second.
>> Boundary is the first to Know...and Tell You.
>> Monitor Your Applications in Ultra-Fine Resolution. Try it
>> FREE!http://p.sf.net/sfu/Boundary-d2dvs2
>>
>>
>>
>> _______________________________________________
>> Ipmitool-devel mailing
>> listIpmitool-devel@lists.sourceforge.nethttps://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>>
>>
>>
>> ------------------------------------------------------------------------------
>> For Developers, A Lot Can Happen In A Second.
>> Boundary is the first to Know...and Tell You.
>> Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
>> http://p.sf.net/sfu/Boundary-d2dvs2
>> _______________________________________________
>> Ipmitool-devel mailing list
>> Ipmitool-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>>
>>
>
0002-ipmi_user.c-uid_checks-v3.diff
Description: Binary data
------------------------------------------------------------------------------ For Developers, A Lot Can Happen In A Second. Boundary is the first to Know...and Tell You. Monitor Your Applications in Ultra-Fine Resolution. Try it FREE! http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
