some safety checks there.
On Thu, Jul 4, 2013 at 9:39 AM, Zdenek Styblik
On Tue, Jul 2, 2013 at 10:41 AM, Dmitry Bazhenov
Post by Dmitry BazhenovHello, Zdenek,
The FRU information sections are hardly a subject for change in the near
future. So, I guess we can skip this issue.
I updated the formatting of the patch according to your comments and removed
the restricted FRU info access entirely since the implemented algorithm
reduces the read/write size while it is not accepted.
Please, review.
Regards,
Dmitry
[...]
Dmitry,
I've given it another look. I've added space here and there and
changed C++ style comments to C style comments. I know you've followed
style in 'lib/ipmi_fru.c', however I think error messages regarding
malloc() failure should be follow the suit of the rest of the code in
ipmitool. Again, easy to fix and somewhat minor. Hell, I'll file new
ticket for it and it will be done later.
I have the following concerns, though. In function build_fru_bloc(),
if malloc() failure occurs, you just break from the loop and that's
about it. I think this is wrong. If malloc() failure occurs, then you
either should return from the function immediately, or set variable
eg. ``error = 1; break;'' and check value of such variable once you
exit the loop.
~~~
for (i = 0; i < 4; i++) {
p_new = malloc(...);
if (p_new == NULL) {
lprintf(LOG_ERR, "out of memory");
break;
}
}
/* nothing to see, nothing just happened, moving on */
[ more code, more malloc() calls etc. ]
~~~
Seems just simply wrong. Application shouldn't continue, in my
opinion, and should terminate. No such thing happens, however, and
function proceeds over malloc() failures like nothing is going on.
fru_area_print_chassis(), fru_area_print_board(),
fru_area_print_product(), fru_area_print_multirec() - similar case.
This function returns void. malloc() failure happens and all we do is
just return from the function? Eh. It just doesn't feel right to me.
Now, I stress it's my opinion application should terminate after
malloc() failure occurs even once. The reason behind this worry is
malloc() failure may occur at given time, t_0, but it doesn't have to
occur later in t_1. Now, there is a bit of inconsistency and perhaps
some NULL pointer, isn't there? Also, more malloc() calls are stacked
up and we've been told once we won't get any memory, so why should we
try again and again? OS can be under heavy load, running short on
resources, yet we'll keep bugging about memory or what not?
However, I'm open to be explained why this isn't necessary or
eventually wrong approach in general. And ok, I can imagine cases why
this is done deliberately, but is it here? Why?
I even offer to put some code behind my words as talk is cheap. I
mean, I'm ok do to changes I've just pointed out and send a patch for
review. If there is no rush as I can't do it right now.
One more thing. If this e-mail feels offensive in any way, it's not
meant to be. So please, take it easy.
Have an easy Monday,
Z.
Post by Dmitry BazhenovPost by Zdenek StyblikOn Thu, Jun 27, 2013 at 6:06 PM, Dmitry Bazhenov
[...]
Post by Dmitry BazhenovPost by Zdenek Styblik1] ``const char *section_id[4]'' - is there no chance at all this is
going to be extended at some point in future? If it is, I'd be for
better solution. My point is ``for (i = 0; i < 4; i++)'' and if it
gets extended so must code which follows/is using/iterate over it. But
may be I'm wrong.
Is 'for ( i = 0; i < sizeof(section_id) / siceof(section_id[0]); i++)'
solves the issue?
Dmitry,
I'm not sure if there was an issue to begin with. But to be honest,
this one looks scary. Please, let me think about it a bit, try
something; I'll try to come up with some constructive(= better?). If I
can't think of anything better, I guess, I shouldn't ask you to. I
mean, I don't know. I'll think about it, try stuff, we'll see, ok? :)
Have a nice weekend,
Z.