On Fri, Apr 26, 2013 at 7:44 AM, Dmitry Bazhenov <dim...@pigeonpoint.com> wrote:
> Hello, Zdenek,
>
> The updated patch to IPMITool works as follows.
>
> By default, ipmishell is enabled (for linux as default OS). Then,
> OS-defaults are applied.
>
> If ipmishell remains enabled by default, its prerequisites are checked and
> if the check fails, then ipmishell is disabled.
>
> Next, the --enable-ipmishell command line parameter is checked (user forces
> build status of ipmishell). If it enables ipmishell, then prerequisites are
> checked once again and if the check fails, the configure script fails as
> well.
>
> I think that this is the most appropriate behavior for configure script
> regarding the ipmishell.
>
> Regards,
> Dmitry
>
Dmitry,
I see. Then I propose to change ipmishell description from 'yes' to
'auto' as well, agreed?
~~~
[AC_HELP_STRING([--enable-ipmishell],
[enable IPMI shell interface [default=yes]])],
~~~
I'd like to omit following change, as I said previously:
~~~
@@ -89,7 +97,6 @@ solaris*)
*freebsd*)
xenable_intf_imb=no
xenable_intf_lipmi=no
- xenable_intf_bmc=no
;;
*netbsd*)
xenable_intf_imb=no
~~~
is that ok?
Thanks,
Z.
> 23.04.2013 14:24, Zdenek Styblik пишет:
>
>> On Tue, Apr 23, 2013 at 6:36 AM, Dmitry Bazhenov <dim...@pigeonpoint.com>
>> wrote:
>>>
>>> Hello, Zdenek,
>>>
>>> Can I treat the patches as accepted or there are going to be other
>>> comments?
>>>
>>> Regards,
>>> Dmitry
>>>
>>
>> Dmitry,
>>
>> the following comments don't necessarily mean you have to re-do
>> things. They can be addressed by person doing commits. Some of them.
>>
>> As for changes to configure, fix for build for the newer autotool
>> release is missing, but I can be scavenged from previous diff. No
>> worries, just don't forget about it. I don't fully agree with removing
>> OS defaults, resp. FreeBSD default of BMC, despite "global" defaults
>> are set above. I have one question about ipmishell and its
>> auto-disable and that's, what if user says he/she wants ipmishell, but
>> doesn't have all necessary prerequisites? Will configure fail or
>> silently disable ipmishell? My guess is it will silently disable
>> ipmishell and I'm not sure if this is a wanted behaviour.
>>
>> Regarding changes to PICMG; if anything, they should be split into
>> two. I want to ask why change formatting at those particular lines,
>> but I guess it doesn't matter that much. As for replacement of
>> atoi()/strto*(), I'd like to work at it a bit. And as I said, these
>> will be tracked under ID#3528310.
>> What I want to change is:
>> * rephrase error messages
>> * move is_fru_id() function from 'ipmi_fru.c' to 'helper.c' and utilize it
>> * I don't know and may not find out what are allowed ranges for LED
>> related stuff, power level, present to desired, control option,
>> resource id etc., but these should be limited as well, not just
>> convert and "forward" to BMC. If you know ranges for any of these,
>> please, share.
>> * additionally replace all atoi()/strto*() calls; that's what
>> ID#3528310 is about anyway.
>>
>> I have no comments on other diffs.
>>
>> My $0.02 USD,
>> Z.
>>
>>> 19.04.2013 12:53, Dmitry Bazhenov пишет:
>>>>
>>>> Hello, Zdenek,
>>>>
>>>> I addressed your comments. Please, review.
>>>> I put the changes into separate files this time.
>>>>
>>>> In configure.ac I made default option values for all interfaces. So,
>>>> there should be no way some of them may be uninitialized.
>>>>
>>>> Also, I re-arranged such that ipmishell is automatically disabled if
>>>> there are no its prerequisites.
>>>>
>>>> In lib_picmg.c I corrected formatting and check all string to integer
>>>> conversions for errors.
>>>>
>>>> Regards,
>>>> Dmitry
>>>>
>>>> 18.04.2013 13:42, Zdenek Styblik пишет:
>>>>>
>>>>> On Thu, Apr 18, 2013 at 7:49 AM, Dmitry Bazhenov
>>>>> <dim...@pigeonpoint.com> wrote:
>>>>>>
>>>>>> Hello, all,
>>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>> The attached patch fixes several compile and run-time bugs in the
>>>>>> current
>>>>>> ipmitool TOB:
>>>>>>>
>>>>>>> o lib/ipmi_sel.c
>>>>>>> - zero data before making request to avoid reading of
>>>>>>> uninitialized data
>>>>>
>>>>>
>>>>> This looks ok.
>>>>>
>>>>>>> o lib/ipmi_mc.c
>>>>>>> - do not print AUX info, if IPMC return 12-byte Get Device ID
>>>>>
>>>>>
>>>>> *shrug*
>>>>>
>>>>>>> response
>>>>>>> o lib/ipmi_picmg.c
>>>>>>> - add support for hexadecimal and octal formats for the
>>>>>>> command-line
>>>>>
>>>>>
>>>>> NACK. Yes, you're adding support for hex and oct, but it doesn't fix
>>>>> possibility of over/under-flow. Please, add this to ID#3528310 and it
>>>>> will get fixed along. Thanks!
>>>>> If you're willing to take it further, please, write a generic function
>>>>> to validate whether FRU ID is within acceptable range and use
>>>>> apropriate str2* functions to convert string to integer. Such thing
>>>>> would be "appreciated".
>>>>>
>>>>>>> parameters
>>>>>>> o configure.in
>>>>>>> - fixed build for the newer autotool releases
>>>>>
>>>>>
>>>>> I guess ACK.
>>>>>
>>>>>>> - do not override OS-default values for IMB, OPEN and LIPMI
>>>>>>> interfaces
>>>>>>> if the corresponding enable/disable arguments are not
>>>>>>> provided in
>>>>>>> the command-line
>>>>>
>>>>>
>>>>> NACK. Have you actually tested results of this patch? The way I see
>>>>> it, fix is to create _os (defaults) variables and then:
>>>>> ``if test "x$xenable_intf_open" = "xyes" || test
>>>>> "x$xenable_intf_open_os" = "xyes"; then''
>>>>>
>>>>> I'll do the patch, eventually, if nobody else does it sooner(faster?).
>>>>>
>>>>> Regards,
>>>>> Z.
>>>>>
>>>>>>
>>>>>>
>>>>>> Please, review.
>>>>>>
>>>>>> Regards,
>>>>>> Dmitry
>>>>>>
>>>>>>
>>>>>>
>>>>>> ------------------------------------------------------------------------------
>>>>>>
>>>>>> Precog is a next-generation analytics platform capable of advanced
>>>>>> analytics on semi-structured data. The platform includes APIs for
>>>>>> building
>>>>>> apps and a phenomenal toolset for data science. Developers can use
>>>>>> our toolset for easy data analysis & visualization. Get a free
>>>>>> account!
>>>>>> http://www2.precog.com/precogplatform/slashdotnewsletter
>>>>>> _______________________________________________
>>>>>> Ipmitool-devel mailing list
>>>>>> Ipmitool-devel@lists.sourceforge.net
>>>>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>>>>>>
>>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Precog is a next-generation analytics platform capable of advanced
>>>> analytics on semi-structured data. The platform includes APIs for
>>>> building
>>>> apps and a phenomenal toolset for data science. Developers can use
>>>> our toolset for easy data analysis & visualization. Get a free account!
>>>> http://www2.precog.com/precogplatform/slashdotnewsletter
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Ipmitool-devel mailing list
>>>> Ipmitool-devel@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>>>>
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Try New Relic Now & We'll Send You this Cool Shirt
>>> New Relic is the only SaaS-based application performance monitoring
>>> service
>>> that delivers powerful full stack analytics. Optimize and monitor your
>>> browser, app, & servers with just a few lines of code. Try New Relic
>>> and get this awesome Nerd Life shirt!
>>> http://p.sf.net/sfu/newrelic_d2d_apr
>>> _______________________________________________
>>> Ipmitool-devel mailing list
>>> Ipmitool-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
