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