Z,
Yes it should return be returning a static char. I'll fix this.
-- 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
>
------------------------------------------------------------------------------
Try before you buy = See our experts in action!
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-dev2
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
