On Mon, Aug 5, 2013 at 10:07 AM, Dmitry Bazhenov <at...@emcraft.com> wrote:
> Hello, Zdenek,
>
> Is there any progress on this ticket. Or there are obstacle for proceeding
> with it?
>
Dmitry,
there is no progress on my side as I had to put ipmitool on
back-burner right now. As for obstacles, if you've addressed my
comments, then I don't know of any other obstacles - I assume I'd not
have any more comments.
I'll try to give your updated patch at least a look. If I see
something, I'll most likely scream.
However, I mean to log ticket for those malloc() calls in those ``just
dumping text'' functions. But as I said previously, that's not your
problem.
Regards,
Z.
> Regards,
> Dmitry
>
> 29.07.2013 12:24, Dmitry Bazhenov пишет:
>
>> Hello, Zdenek,
>>
>> I addressed the comment about malloc() failures in the build_fru_bloc()
>> function. Now it aborts the operation on any failure.
>>
>> I didn't address the other malloc() failures since the spoken functions
>> are just dump the text.
>>
>> Regards,
>> Dmitry
>>
>> 15.07.2013 15:57, Zdenek Styblik пишет:
>>>
>>> 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.
>>>>>>>>
>>>>>>>
>>>>
>
------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead.
Download for free and get started troubleshooting in minutes.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
