On Thu, Jul 4, 2013 at 9:39 AM, Zdenek Styblik <zdenek.styb...@gmail.com> wrote:
> On Tue, Jul 2, 2013 at 10:41 AM, Dmitry Bazhenov <dim...@pigeonpoint.com>
> wrote:
>> Hello, 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.
Example:
~~~
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.
>> 28.06.2013 18:51, Zdenek Styblik пишет:
>>
>>> On Thu, Jun 27, 2013 at 6:06 PM, Dmitry Bazhenov <dim...@pigeonpoint.com>
>>> wrote:
>>> [...]
>>>>>
>>>>> 1] ``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.
>>>
>>
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
