Z,
I can fix the indentation any way you would like, but I need to know
exactly 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.
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?
-- 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
>>>
>
------------------------------------------------------------------------------
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
