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
<mailto: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 <mailto: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
<mailto: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 list
Ipmitool-devel@lists.sourceforge.net
<mailto:Ipmitool-devel@lists.sourceforge.net>
https://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
<mailto:Ipmitool-devel@lists.sourceforge.net>
https://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
