Answers inline. On Tue, Feb 14, 2012 at 8:21 PM, Jim Mank <jm...@hp.com> wrote: > Z, > I can fix the indentation any way you would like, but I need to know exactly
I feel like this is "how to please me". Nothing like that should be the case. > how the indentation should be done. I generally indent using tabs (with 8 > chars per tab stop) adding extra spaces after the tabs to do more specific > indentation for lines which are longer than 80 characters. I think it is > the extra spaces that you are referring to as they won't line up correctly > with less than 8 characters per tab stop. I agree with tabs and 80 chars max per line. As for tabs and spaces. I, personally, using only tabs without any spaces, but I've seen formatting you're referring to and it is more likely I'm just doing it wrong. Anyway, as long as tabs and spaces(as main indentation) do not mix in one file, it is ok. However, that's not the case of ipmitool as of now. The line I was picking at is line 211(not 210) in patch/diff you sent. Just check the file. There is a line with one tab followed by many spaces and it seem to be wrong to me. > After I get this indentation straightened out, how do I go about getting > this change into the CVS repository and how do I go about closing out the > defect? > As mentioned at SF.net: 1] checkout ipmitool's CVS(substitute what's necessary): % export CVS_RSH=ssh ; % cvs -z3 -d:ext:developern...@ipmitool.cvs.sourceforge.net:/cvsroot/ipmitool co -P modulename ; 2] patch it 3] review changes: % cvs diff -u <files> ; 3] commit: % cvs commit <files> ; * if you are behind firewall/proxy, you may need: https://wiki.zeratul.org/doku.php?id=linux:scribble#cvs_via_ssh_behind_firewall-proxy * it is not a bad idea to set your favorite editor: % export CVSEDITOR=/usr/bin/vim ; * you can use '-m <message_file>' to read commit message from file I guess that should cover it. And continue further development on up-to-date CVS version(?). Z. > > -- Jim Mankovich | jm...@hp.com -- > > > On 2/14/2012 2:40 AM, Zdenek Styblik wrote: >> >> Jim, >> >> I'm sorry for delayed reply, but I didn't feel well yesterday(not like >> it is better today). >> >> Line 210 in patch file seems to have indentations off resp. there are >> tabs and then spaces which doesn't align to above. >> I can't think of anything else looking at the diff. Perhaps >> indentations here and there, but I don't know(nit picking). >> >> Regards, >> Z. >> >> On Mon, Feb 13, 2012 at 3:36 PM, Jim Mank<jm...@hp.com> wrote: >>> >>> Z, >>> >>> Here is the external include file declaration, and the new function >>> declaration in the source file to cover the fact that the function >>> returns a >>> pointer to a static character array. >>> >>> I have attached a patch file with my complete set of changes. Let me >>> know >>> if there is anything else I missed. >>> >>> const char *ipmi_sdr_get_unit_string(uint8_t pct, uint8_t type, >>> uint8_t base, uint8_t modifier); >>> >>> const char * >>> >>> ipmi_sdr_get_unit_string(uint8_t pct, uint8_t type, uint8_t base, uint8_t >>> modifier) >>> { >>> static char unitstr[16]; >>> /* >>> * By default, if units are supposed to be percent, we will pre-pend >>> * the percent string to the textual representation of the units. >>> */ >>> char *pctstr = pct ? "% " : ""; >>> memset(unitstr, 0, sizeof (unitstr)); >>> switch (type) { >>> case 2: >>> snprintf(unitstr, sizeof (unitstr), "%s%s * %s", >>> pctstr, unit_desc[base], unit_desc[modifier]); >>> break; >>> case 1: >>> snprintf(unitstr, sizeof (unitstr), "%s%s/%s", >>> pctstr, unit_desc[base], unit_desc[modifier]); >>> break; >>> case 0: >>> default: >>> /* >>> * Display the text "percent" only when the Base unit is >>> * "unspecified" and the caller specified to print percent. >>> */ >>> if (base == 0&& pct) { >>> snprintf(unitstr, sizeof(unitstr), "percent"); >>> } else { >>> snprintf(unitstr, sizeof (unitstr), "%s%s", >>> pctstr, unit_desc[base]); >>> } >>> break; >>> } >>> >>> return unitstr; >>> } >>> >>> -- Jim Mankovich | jm...@hp.com -- >>> >>> >>> On 2/9/2012 10:18 PM, Zdenek Styblik wrote: >>>> >>>> Hi, >>>> >>>> it looks fine by me. One thing though. Since it returns pointer to >>>> static char, should it be: >>>> >>>> static char * >>>> ipmi_sdr_get_unit_string(uint8_t pct, uint8_t type, uint8_t base, >>>> uint8_t modifier) >>>> { ... } >>>> >>>> as well? >>>> >>>> Regards, >>>> Z. >>>> >>>> On Thu, Feb 9, 2012 at 10:01 PM, Jim Mank<jm...@hp.com> wrote: >>>> [...] >>>>> >>>>> Any/All comments and questions would be much appreciated. >>>>> >>>>> /* ipmi_sdr_get_unit_string - return units for base/modifier >>>>> * >>>>> * @pct: units are a percentage >>>>> * @type: unit type >>>>> * @base: base >>>>> * @modifier: modifier >>>>> * >>>>> * returns pointer to static string >>>>> */ >>>>> char * >>>>> ipmi_sdr_get_unit_string(uint8_t pct, uint8_t type, uint8_t base, >>>>> uint8_t modifier) >>>>> { >>>>> static char unitstr[16]; >>>>> /* >>>>> * By default, if units are supposed to be percent, we will >>>>> pre-pend >>>>> * the percent string to the textual representation of the >>>>> units. >>>>> */ >>>>> char *pctstr = pct ? "% " : ""; >>>>> memset(unitstr, 0, sizeof (unitstr)); >>>>> switch (type) { >>>>> case 2: >>>>> snprintf(unitstr, sizeof (unitstr), "%s%s * %s", >>>>> pctstr, unit_desc[base], unit_desc[modifier]); >>>>> break; >>>>> case 1: >>>>> snprintf(unitstr, sizeof (unitstr), "%s%s/%s", >>>>> pctstr, unit_desc[base], unit_desc[modifier]); >>>>> break; >>>>> case 0: >>>>> default: >>>>> /* >>>>> * Display the text "percent" only when the Base unit >>>>> is >>>>> * "unspecified" and the caller specified to print >>>>> percent. >>>>> */ >>>>> if (base == 0&& pct) { >>>>> >>>>> snprintf(unitstr, sizeof(unitstr), "percent"); >>>>> } else { >>>>> snprintf(unitstr, sizeof (unitstr), "%s%s", >>>>> pctstr, unit_desc[base]); >>>>> } >>>>> break; >>>>> } >>>>> >>>>> return unitstr; >>>>> } >>>>> >>>>> -- Jim Mankovich | jm...@hp.com -- >>>>> >>>>> On 2/8/2012 12:04 PM, Jim Mank wrote: >>>>>> >>>>>> Al, >>>>>> >>>>>> It does appear from the spec that percent can be applied to any >>>>>> validly >>>>>> specified Unit given that is an independent bit that can be specified >>>>>> in >>>>>> addition to the Base/Modifier units. I could not find any other text >>>>>> about the percent interpretation. >>>>>> >>>>>> If this is the spec intent, then when the percent bit is a one and >>>>>> the >>>>>> Base Unit is "unspecified", a textural string of "percent" or "%" >>>>>> could >>>>>> be used as the display string for the units. This would directly >>>>>> address what was reported as the bug, but would not address what you >>>>>> pointed out. This is the only case I took into account with I have >>>>>> done because the interpretation of the pct bit was not clearly >>>>>> articulated in the spec. >>>>>> >>>>>> When the percent bit is a one and the Base Unit has a value != 0, then >>>>>> pre-pending the string "percent" or "%" to the Base Unit makes sense >>>>>> to >>>>>> me. I specifically avoided talking about applying percent to the >>>>>> Modifier Unit since it only seems to make sense to apply percent to >>>>>> the >>>>>> entire entity when both Base and Modifier are specified. >>>>>> >>>>>> I do have some concern that decoding the percent bit as being applied >>>>>> to any valid Base Unit specification could cause problems if platform >>>>>> IPMI implementations were done that did not interpret the percent bit >>>>>> in >>>>>> exactly the way we are talking about it. >>>>>> >>>>>> -- Jim Mankovich | jm...@hp.com -- >>>>>> >>>>>> >>>>>> On 2/8/2012 11:01 AM, Albert Chu wrote: >>>>>>> >>>>>>> Hi Jim, >>>>>>> >>>>>>> On Wed, 2012-02-08 at 08:37 -0800, Jim Mank wrote: >>>>>>>> >>>>>>>> All, >>>>>>>> >>>>>>>> I would like to start contributing to the ipmitool project and as a >>>>>>>> initial task I thought I would resolve the open bug associated with >>>>>>>> ipmitool's inability to display percentage units correctly >>>>>>>> >>>>>>>> >>>>>>>> (https://sourceforge.net/tracker/?func=detail&aid=3014014&group_id=95200&atid=610550). >>>>>>>> I'm currently working for Hewlett Packard in their Proliant server >>>>>>>> organization and I and others are starting to look at how we can >>>>>>>> better >>>>>>>> serve our customers by helping make ipmitool more robust. I only >>>>>>>> have >>>>>>>> access to HP Proliant servers, so I'll be doing my testing on >>>>>>>> various >>>>>>>> HP >>>>>>>> Proliant servers. I'm not very familiar with IPMI so I'll be >>>>>>>> learning >>>>>>>> as I work though problems, any and all help with IPMI would much >>>>>>>> appreciated. >>>>>>>> >>>>>>>> In investigating this problem, I found in the IPMI v2.0 spec that >>>>>>>> the >>>>>>>> percentage units are identified by bit 0 == 1 in byte 21 of the Full >>>>>>>> and >>>>>>>> Compact Sensor Records. In looking at the ipmitool code, the >>>>>>>> sdr_record_full_sensor and sdr_record_compact_sensor both properly >>>>>>>> declare the percentage bit (pct), but no code looks at this. So, >>>>>>>> display of percentage units correctly in the ipmitool requires >>>>>>>> correct >>>>>>>> interpretation of this bit in the sensor records. As I understand >>>>>>>> the >>>>>>>> spec, if the pct bit is set to one, then the units are a percentage >>>>>>>> and >>>>>>>> the Base Unit would be unspecified. >>>>>>> >>>>>>> While this is the 99.9% case, I believe it is legal for the base unit >>>>>>> to >>>>>>> be specified. Looking through the units list "% hit" and "% miss" >>>>>>> seem >>>>>>> like reasonable base units to go with "%". There might be other >>>>>>> possibilities when you add the rate unit and modifier units (i.e. "% >>>>>>> X >>>>>>> / >>>>>>> Y" or "% X per Y"). >>>>>>> >>>>>>> My code here is probably not perfect, but perhaps this can give you >>>>>>> hints on some of the corner cases that can be expected: >>>>>>> >>>>>>> >>>>>>> >>>>>>> http://svn.savannah.gnu.org/viewvc/trunk/libfreeipmi/util/ipmi-sensor-units-util.c?revision=8165&root=freeipmi&view=markup >>>>>>> >>>>>>> Al >>>>>>> >>>>>>>> The code for displaying units in the sensor records is replicated in >>>>>>>> multiple places in the code, each doing pretty much the exact same >>>>>>>> decoding. There does exist one function for decoding the units, >>>>>>>> ipmi_sdr_get_unit_string, so I've changed the code so this routine >>>>>>>> will >>>>>>>> be used for all units decoding and I updated this function to >>>>>>>> display >>>>>>>> a >>>>>>>> new unit named "percent". >>>>>>>> >>>>>>>> In doing these modifications, I'm wondering if it would be ok for me >>>>>>>> to >>>>>>>> to do some local variable cleanup in ipmi_sdr_print_sensor_full. I >>>>>>>> noticed that the local variable do_unit is set to one, but never >>>>>>>> changed, so could this code be removed? >>>>>>>> >>>>>>>> When I have these changes available, should I just send a patch to >>>>>>>> this >>>>>>>> email alias? >>>>>>>> >>>>>> >>>>>> ------------------------------------------------------------------------------ >>>>>> Keep Your Developer Skills Current with LearnDevNow! >>>>>> The most comprehensive online learning library for Microsoft >>>>>> developers >>>>>> is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, >>>>>> MVC3, >>>>>> Metro Style Apps, more. Free future releases when you subscribe now! >>>>>> http://p.sf.net/sfu/learndevnow-d2d >>>>>> _______________________________________________ >>>>>> Ipmitool-devel mailing list >>>>>> Ipmitool-devel@lists.sourceforge.net >>>>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel >>>>>> >>>>> >>>>> ------------------------------------------------------------------------------ >>>>> Virtualization& Cloud Management Using Capacity Planning >>>>> >>>>> Cloud computing makes use of virtualization - but cloud computing >>>>> also focuses on allowing computing to be delivered as a service. >>>>> http://www.accelacomm.com/jaw/sfnl/114/51521223/ >>>>> _______________________________________________ >>>>> Ipmitool-devel mailing list >>>>> Ipmitool-devel@lists.sourceforge.net >>>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel >>>> >>>> >> > ------------------------------------------------------------------------------ Virtualization & Cloud Management Using Capacity Planning Cloud computing makes use of virtualization - but cloud computing also focuses on allowing computing to be delivered as a service. http://www.accelacomm.com/jaw/sfnl/114/51521223/ _______________________________________________ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
