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
