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 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
