On Mon, Jul 15, 2013 at 10:50 AM, Dmitry Bazhenov
<dim...@pigeonpoint.com> wrote:
> Hello, Zdenek,
>
> Thanks for review.
> I see your point and I think it make sense. I am going to address your
> comments regarding the build_fru_bloc() function since this one was under my
> heavy changes.
Ok, great.
> I will also look at the other functions
> (fru_area_print_chassis(), fru_area_print_board(),
> fru_area_print_product(), fru_area_print_multirec()), but I think they are
> of different case since they are just dumping functions. And in my opinion
> it is not of big deal to skip through some memory allocation failures.
*shrug* I somewhat knew it's coming, but I've failed to address it.
Yes, they are just dumping/printing data, but they're not much of
different than any other function. It may not matter in this
case/context, but it could matter in different one ...:
~~~
[ some code here ]
fru_area_print_chassis()
[ some more code here ]
~~~
Actually, these 4 functions in question are called in consecutive order.
Hm, how about this. I'll log tickets for those and it'll get
fixed(meaning I'll fix it) later. Now I hate myself for writing, even
thinking of, such thing.
The basic idea I have is to return int and check it. No biggie.
Z.
> However, if it leads to dereference of NULL, then it is important to add
> some safety checks there.
>
> Will such a plan work for you?
>
> Regards,
> Dmitry
>
> 15.07.2013 14:01, Zdenek Styblik пишет:
>
>> 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
