Hello, Zdenek, No objections.
Dmitry 26.04.2013 23:10, Zdenek Styblik пишет: > 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
