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
diff --git a/include/ipmitool/ipmi_sdr.h b/include/ipmitool/ipmi_sdr.h
index 7dd1676..d51a9bb 100644
--- a/include/ipmitool/ipmi_sdr.h
+++ b/include/ipmitool/ipmi_sdr.h
@@ -896,7 +896,8 @@ int ipmi_sdr_print_rawentry(struct ipmi_intf *intf, uint8_t type, uint8_t * raw,
int len);
int ipmi_sdr_print_listentry(struct ipmi_intf *intf,
struct sdr_record_list *entry);
-char *ipmi_sdr_get_unit_string(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);
const char *ipmi_sdr_get_status(struct sdr_record_full_sensor *sensor,
uint8_t stat);
double sdr_convert_sensor_tolerance(struct sdr_record_full_sensor *sensor,
diff --git a/lib/ipmi_sdr.c b/lib/ipmi_sdr.c
index 381649b..86ab1d0 100644
--- a/lib/ipmi_sdr.c
+++ b/lib/ipmi_sdr.c
@@ -64,30 +64,44 @@ static struct ipmi_sdr_iterator *sdr_list_itr = NULL;
/* 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 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",
- unit_desc[base], unit_desc[modifier]);
+ snprintf(unitstr, sizeof (unitstr), "%s%s * %s",
+ pctstr, unit_desc[base], unit_desc[modifier]);
break;
case 1:
- snprintf(unitstr, sizeof (unitstr), "%s/%s",
- unit_desc[base], unit_desc[modifier]);
+ snprintf(unitstr, sizeof (unitstr), "%s%s/%s",
+ pctstr, unit_desc[base], unit_desc[modifier]);
break;
case 0:
default:
- snprintf(unitstr, sizeof (unitstr), "%s", unit_desc[base]);
+ /*
+ * 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;
}
@@ -1154,8 +1168,9 @@ int
ipmi_sdr_print_sensor_full(struct ipmi_intf *intf,
struct sdr_record_full_sensor *sensor)
{
- char sval[16], unitstr[16], desc[17];
- int i = 0, validread = 1, do_unit = 1;
+ const char *unitstr = NULL;
+ char sval[16], desc[17];
+ int i = 0, validread = 1;
double val = 0.0, creading = 0.0;
struct ipmi_rs *rsp;
uint8_t target, lun, channel;
@@ -1219,25 +1234,11 @@ ipmi_sdr_print_sensor_full(struct ipmi_intf *intf,
}
/* determine units with possible modifiers */
- if (do_unit && validread) {
- memset(unitstr, 0, sizeof (unitstr));
- switch (sensor->unit.modifier) {
- case 2:
- i += snprintf(unitstr, sizeof (unitstr), "%s * %s",
- unit_desc[sensor->unit.type.base],
- unit_desc[sensor->unit.type.modifier]);
- break;
- case 1:
- i += snprintf(unitstr, sizeof (unitstr), "%s/%s",
- unit_desc[sensor->unit.type.base],
- unit_desc[sensor->unit.type.modifier]);
- break;
- case 0:
- default:
- i += snprintf(unitstr, sizeof (unitstr), "%s",
- unit_desc[sensor->unit.type.base]);
- break;
- }
+ if (validread) {
+ unitstr = ipmi_sdr_get_unit_string(sensor->unit.pct,
+ sensor->unit.modifier,
+ sensor->unit.type.base,
+ sensor->unit.type.modifier);
}
/*
@@ -1252,7 +1253,7 @@ ipmi_sdr_print_sensor_full(struct ipmi_intf *intf,
if (validread) {
printf("%.*f,", (val == (int) val) ? 0 : 3, val);
- printf("%s,%s", do_unit ? unitstr : "",
+ printf("%s,%s", unitstr,
ipmi_sdr_get_status(sensor, rsp->data[2]));
} else {
printf(",,ns");
@@ -1310,8 +1311,7 @@ ipmi_sdr_print_sensor_full(struct ipmi_intf *intf,
if (validread)
i += snprintf(sval, sizeof (sval), "%.*f %s",
- (val == (int) val) ? 0 : 2, val,
- do_unit ? unitstr : "");
+ (val == (int) val) ? 0 : 2, val, unitstr);
else if (rsp && IS_SCANNING_DISABLED(rsp->data[1]))
i += snprintf(sval, sizeof (sval), "disabled ");
else
@@ -1344,8 +1344,7 @@ ipmi_sdr_print_sensor_full(struct ipmi_intf *intf,
if (validread)
i += snprintf(sval, sizeof (sval), "%.*f %s",
- (val == (int) val) ? 0 : 2, val,
- do_unit ? unitstr : "");
+ (val == (int) val) ? 0 : 2, val, unitstr);
else if (rsp && IS_SCANNING_DISABLED(rsp->data[1]))
i += snprintf(sval, sizeof (sval), "Disabled");
else
diff --git a/lib/ipmi_sel.c b/lib/ipmi_sel.c
index bfb27c4..3a3037a 100644
--- a/lib/ipmi_sel.c
+++ b/lib/ipmi_sel.c
@@ -1124,7 +1124,8 @@ ipmi_sel_print_std_entry(struct ipmi_intf * intf, struct sel_event_record * evt)
((evt->sel_type.standard_type.event_data[0] & 0xf) % 2) ? ">" : "<",
(threshold_reading==(int)threshold_reading) ? 0 : 2,
threshold_reading,
- ipmi_sdr_get_unit_string(sdr->record.full->unit.modifier,
+ ipmi_sdr_get_unit_string(sdr->record.full->unit.pct,
+ sdr->record.full->unit.modifier,
sdr->record.full->unit.type.base,
sdr->record.full->unit.type.modifier));
}
@@ -1311,25 +1312,10 @@ ipmi_sel_print_extended_entry_verbose(struct ipmi_intf * intf, struct sel_event_
sdr_convert_sensor_reading(sdr->record.full,
evt->sel_type.standard_type.event_data[1]));
/* determine units with possible modifiers */
- switch (sdr->record.full->unit.modifier) {
- case 2:
- printf(" %s * %s\n",
- unit_desc[sdr->record.full->unit.type.base],
- unit_desc[sdr->record.full->unit.type.modifier]);
- break;
- case 1:
- printf(" %s/%s\n",
- unit_desc[sdr->record.full->unit.type.base],
- unit_desc[sdr->record.full->unit.type.modifier]);
- break;
- case 0:
- printf(" %s\n",
- unit_desc[sdr->record.full->unit.type.base]);
- break;
- default:
- printf("\n");
- break;
- }
+ printf ("%s\n", ipmi_sdr_get_unit_string(sdr->record.full->unit.pct,
+ sdr->record.full->unit.modifier,
+ sdr->record.full->unit.type.base,
+ sdr->record.full->unit.type.modifier));
break;
case 2:
/* oem code in byte 2 */
@@ -1352,25 +1338,10 @@ ipmi_sel_print_extended_entry_verbose(struct ipmi_intf * intf, struct sel_event_
sdr_convert_sensor_reading(sdr->record.full,
evt->sel_type.standard_type.event_data[2]));
/* determine units with possible modifiers */
- switch (sdr->record.full->unit.modifier) {
- case 2:
- printf(" %s * %s\n",
- unit_desc[sdr->record.full->unit.type.base],
- unit_desc[sdr->record.full->unit.type.modifier]);
- break;
- case 1:
- printf(" %s/%s\n",
- unit_desc[sdr->record.full->unit.type.base],
- unit_desc[sdr->record.full->unit.type.modifier]);
- break;
- case 0:
- printf(" %s\n",
- unit_desc[sdr->record.full->unit.type.base]);
- break;
- default:
- printf("\n");
- break;
- }
+ printf ("%s\n", ipmi_sdr_get_unit_string(sdr->record.full->unit.pct,
+ sdr->record.full->unit.modifier,
+ sdr->record.full->unit.type.base,
+ sdr->record.full->unit.type.modifier));
break;
case 2:
/* OEM code in byte 3 */
diff --git a/lib/ipmi_sensor.c b/lib/ipmi_sensor.c
index 52ea89a..58b793b 100644
--- a/lib/ipmi_sensor.c
+++ b/lib/ipmi_sensor.c
@@ -228,8 +228,9 @@ static int
ipmi_sensor_print_full_analog(struct ipmi_intf *intf,
struct sdr_record_full_sensor *sensor)
{
- char unitstr[16], id[17];
- int i = 0, validread = 1, thresh_available = 1;
+ const char *unitstr = NULL;
+ char id[17];
+ int validread = 1, thresh_available = 1;
double val = 0.0;
struct ipmi_rs *rsp;
char *status = NULL;
@@ -277,25 +278,10 @@ ipmi_sensor_print_full_analog(struct ipmi_intf *intf,
/*
* Figure out units
*/
- memset(unitstr, 0, sizeof (unitstr));
- switch (sensor->unit.modifier) {
- case 2:
- i += snprintf(unitstr, sizeof (unitstr), "%s * %s",
- unit_desc[sensor->unit.type.base],
- unit_desc[sensor->unit.type.modifier]);
- break;
- case 1:
- i += snprintf(unitstr, sizeof (unitstr), "%s/%s",
- unit_desc[sensor->unit.type.base],
- unit_desc[sensor->unit.type.modifier]);
- break;
- case 0:
- default:
- i += snprintf(unitstr, sizeof (unitstr), "%s",
- unit_desc[sensor->unit.type.base]);
- break;
- }
-
+ unitstr = ipmi_sdr_get_unit_string(sensor->unit.pct,
+ sensor->unit.modifier,
+ sensor->unit.type.base,
+ sensor->unit.type.modifier);
/*
* Get sensor thresholds
*/
diff --git a/src/ipmievd.c b/src/ipmievd.c
index 422fcda..2512250 100644
--- a/src/ipmievd.c
+++ b/src/ipmievd.c
@@ -284,7 +284,8 @@ log_event(struct ipmi_event_intf * eintf, struct sel_event_record * evt)
((evt->sel_type.standard_type.event_data[0] & 0xf) % 2) ? ">" : "<",
(threshold_reading==(int)threshold_reading) ? 0 : 2,
threshold_reading,
- ipmi_sdr_get_unit_string(sdr->record.full->unit.modifier,
+ ipmi_sdr_get_unit_string(sdr->record.full->unit.pct,
+ sdr->record.full->unit.modifier,
sdr->record.full->unit.type.base,
sdr->record.full->unit.type.modifier));
}
------------------------------------------------------------------------------
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
