Discussion:
[Ipmitool-devel] [PATCH 1/3] Fixed help for sdr command.
Dan Gora
2013-03-22 00:15:45 UTC
Permalink
Fixed the formatting of the help command to make it clearer what
options the sdr command has.

Signed-off-by: Dan Gora <***@adax.com>
---
ipmitool/lib/ipmi_sdr.c | 71 +++++++++++++++++++++++++++++------------------
1 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/ipmitool/lib/ipmi_sdr.c b/ipmitool/lib/ipmi_sdr.c
index 698f051..272dee0 100644
--- a/ipmitool/lib/ipmi_sdr.c
+++ b/ipmitool/lib/ipmi_sdr.c
@@ -3743,7 +3743,7 @@ ipmi_sdr_find_sdr_byid(struct ipmi_intf *intf, char *id)
}

/* now keep looking */
- while ((header = ipmi_sdr_get_next_header(intf, sdr_list_itr)) != NULL) {
+ while ((header = ipmi_sdr_get_next_header(intf, sdr_list_itr)) != NULL){
uint8_t *rec;
struct sdr_record_list *sdrr;

@@ -4406,6 +4406,7 @@ ipmi_sdr_print_type(struct ipmi_intf *intf, char *type)
}
}
if (sensor_type != x) {
+ printf("Sensor Type %s not found.\n", type);
printf("Sensor Types:\n");
for (x = 1; x < SENSOR_TYPE_MAX; x += 2) {
printf("\t%-25s %-25s\n",
@@ -4552,49 +4553,65 @@ ipmi_sdr_main(struct ipmi_intf *intf, int argc, char **argv)
if (argc == 0)
return ipmi_sdr_print_sdr(intf, 0xfe);
else if (strncmp(argv[0], "help", 4) == 0) {
+ lprintf(LOG_ERR, "SDR Commands:");
lprintf(LOG_ERR,
- "SDR Commands: list | elist [all|full|compact|event|mcloc|fru|generic]");
+ " list | elist [option]. Available options are:");
lprintf(LOG_ERR,
- " all All SDR Records");
+ " all All SDR Records");
lprintf(LOG_ERR,
- " full Full Sensor Record");
+ " full Full Sensor Record");
lprintf(LOG_ERR,
- " compact Compact Sensor Record");
+ " compact Compact Sensor Record");
lprintf(LOG_ERR,
- " event Event-Only Sensor Record");
+ " event Event-Only Sensor Record");
lprintf(LOG_ERR,
- " mcloc Management Controller Locator Record");
+ " mcloc Management Controller Locator Record");
lprintf(LOG_ERR,
- " fru FRU Locator Record");
+ " fru FRU Locator Record");
lprintf(LOG_ERR,
- " generic Generic Device Locator Record");
- lprintf(LOG_ERR, " type [sensor type]");
+ " generic Generic Device Locator Record");
+ lprintf(LOG_ERR,
+ " type [sensor type] | list");
lprintf(LOG_ERR,
- " list Get a list of available sensor types");
+ " Retrieve sensors matching [sensor type]");
lprintf(LOG_ERR,
- " get Retrieve the state of a specified sensor");
-
- lprintf(LOG_ERR, " info");
+ " list Get a list of available sensor types");
lprintf(LOG_ERR,
- " Display information about the repository itself");
- lprintf(LOG_ERR, " entity <id>[.<instance>]");
+ " get");
lprintf(LOG_ERR,
- " Display all sensors associated with an entity");
- lprintf(LOG_ERR, " dump <file>");
+ " Retrieve the state of a specified sensor");
+ lprintf(LOG_ERR,
+ " info");
lprintf(LOG_ERR,
- " Dump raw SDR data to a file");
- lprintf(LOG_ERR, " fill");
+ " Display information about the repository itself");
+ lprintf(LOG_ERR,
+ " entity <id>[.<instance>]");
lprintf(LOG_ERR,
- " sensors Creates the SDR repository for the current configuration");
+ " Display all sensors associated with an entity");
+ lprintf(LOG_ERR,
+ " dump <file>");
lprintf(LOG_ERR,
- " nosat Creates the SDR repository for the current configuration, without satellite scan");
-
+ " Dump raw SDR data to a file");
+ lprintf(LOG_ERR,
+ " fill [option]. Available options:");
+ lprintf(LOG_ERR,
+ " sensors Creates the SDR repository for the current");
+ lprintf(LOG_ERR,
+ " configuration.");
+ lprintf(LOG_ERR,
+ " nosat Creates the SDR repository for the current");
+ lprintf(LOG_ERR,
+ " configuration, without satellite scan.");
+ lprintf(LOG_ERR,
+ " file <file> Load SDR repository from a file");
+ lprintf(LOG_ERR,
+ " range <range> Load SDR repository from a provided list");
lprintf(LOG_ERR,
- " file <file> Load SDR repository from a file");
+ " or range");
lprintf(LOG_ERR,
- " range <range> Load SDR repository from a provided list or range");
+ " Use , for list or - for range");
lprintf(LOG_ERR,
- " - Use , for list or - for range (Ex.: 0x28,0x32,0x40-0x44) ");
+ " (Ex.: 0x28,0x32,0x40-0x44) ");
} else if (strncmp(argv[0], "list", 4) == 0
|| strncmp(argv[0], "elist", 5) == 0) {

@@ -4661,7 +4678,7 @@ ipmi_sdr_main(struct ipmi_intf *intf, int argc, char **argv)
rc = -1;
} else if (strncmp(argv[1], "sensors", 7) == 0) {
rc = ipmi_sdr_add_from_sensors(intf, 21);
- } else if (strncmp(argv[1], "nosats", 6) == 0) {
+ } else if (strncmp(argv[1], "nosat", 5) == 0) {
rc = ipmi_sdr_add_from_sensors(intf, 0);
} else if (strncmp(argv[1], "file", 4) == 0) {
if (argc < 3) {
--
1.7.7
Dan Gora
2013-03-22 00:15:46 UTC
Permalink
Fix the help for the sdr get command to make it clear that the user
needs to specify the sensor ID string and that it only returns the
_first_ sensor that matches, not all of them.

Fix the help for the sdt type command to make it clear that the type
can be specified as either hex or the string value for the type.
Add the hex value for the sensor type to displays so that the user
can correlate sensor type values with their hex values without having
to look them up in the IPMI spec.

Signed-off-by: Dan Gora <***@adax.com>
---
ipmitool/lib/ipmi_sdr.c | 36 ++++++++++++++++++++++--------------
1 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/ipmitool/lib/ipmi_sdr.c b/ipmitool/lib/ipmi_sdr.c
index 272dee0..63dfced 100644
--- a/ipmitool/lib/ipmi_sdr.c
+++ b/ipmitool/lib/ipmi_sdr.c
@@ -1702,8 +1702,9 @@ ipmi_sdr_print_sensor_fc(struct ipmi_intf *intf,

if (!IS_THRESHOLD_SENSOR(sensor)) {
/* Discrete */
- printf(" Sensor Type (Discrete): %s\n",
- ipmi_sdr_get_sensor_type_desc(sensor->sensor.type));
+ printf(" Sensor Type (Discrete): %s (0x%02x)\n",
+ ipmi_sdr_get_sensor_type_desc(sensor->sensor.type),
+ sensor->sensor.type);
lprintf(LOG_DEBUG, " Event Type Code : 0x%02x",
sensor->event_type);

@@ -1763,8 +1764,9 @@ ipmi_sdr_print_sensor_fc(struct ipmi_intf *intf,

return 0; /* done */
}
- printf(" Sensor Type (Threshold) : %s\n",
- ipmi_sdr_get_sensor_type_desc(sensor->sensor.type));
+ printf(" Sensor Type (Threshold) : %s (0x%02x)\n",
+ ipmi_sdr_get_sensor_type_desc(sensor->sensor.type),
+ sensor->sensor.type);

printf(" Sensor Reading : ");
if (sr->s_reading_valid) {
@@ -2087,8 +2089,9 @@ ipmi_sdr_print_sensor_eventonly(struct ipmi_intf *intf,
printf("Entity ID : %d.%d (%s)\n",
sensor->entity.id, sensor->entity.instance,
val2str(sensor->entity.id, entity_id_vals));
- printf("Sensor Type : %s\n",
- ipmi_sdr_get_sensor_type_desc(sensor->sensor_type));
+ printf("Sensor Type : %s (0x%02x)\n",
+ ipmi_sdr_get_sensor_type_desc(sensor->sensor_type),
+ sensor->sensor_type);
lprintf(LOG_DEBUG, "Event Type Code : 0x%02x",
sensor->event_type);
printf("\n");
@@ -4382,8 +4385,9 @@ ipmi_sdr_print_type(struct ipmi_intf *intf, char *type)
strncasecmp(type, "list", 4) == 0) {
printf("Sensor Types:\n");
for (x = 1; x < SENSOR_TYPE_MAX; x += 2) {
- printf("\t%-25s %-25s\n",
- sensor_type_desc[x], sensor_type_desc[x + 1]);
+ printf("\t%-25s (0x%02x) %-25s (0x%02x)\n",
+ sensor_type_desc[x], x,
+ sensor_type_desc[x + 1], x + 1);
}
return 0;
}
@@ -4409,9 +4413,9 @@ ipmi_sdr_print_type(struct ipmi_intf *intf, char *type)
printf("Sensor Type %s not found.\n", type);
printf("Sensor Types:\n");
for (x = 1; x < SENSOR_TYPE_MAX; x += 2) {
- printf("\t%-25s %-25s\n",
- sensor_type_desc[x],
- sensor_type_desc[x + 1]);
+ printf("\t%-25s (0x%02x) %-25s (0x%02x)\n",
+ sensor_type_desc[x], x,
+ sensor_type_desc[x + 1], x + 1);
}
return 0;
}
@@ -4573,13 +4577,17 @@ ipmi_sdr_main(struct ipmi_intf *intf, int argc, char **argv)
lprintf(LOG_ERR,
" type [sensor type] | list");
lprintf(LOG_ERR,
- " Retrieve sensors matching [sensor type]");
+ " Retrieve sensors matching [sensor type].");
+ lprintf(LOG_ERR,
+ " [sensor type] can be specified as hex value or a string.");
lprintf(LOG_ERR,
" list Get a list of available sensor types");
lprintf(LOG_ERR,
- " get");
+ " get [Sensor ID string]");
+ lprintf(LOG_ERR,
+ " Retrieve the first sensor with Sensor ID matching");
lprintf(LOG_ERR,
- " Retrieve the state of a specified sensor");
+ " [Sensor ID string].");
lprintf(LOG_ERR,
" info");
lprintf(LOG_ERR,
--
1.7.7
Dan Gora
2013-03-22 00:15:47 UTC
Permalink
Previous versions had two values for "Watchdog" in the list which
means that users could not perform 'ipmitool sdr type "Watchdog"'
and get the IPMI v1.0 and later version of the watchdog sensor (ie
0x23 instead of 0x11).

Signed-off-by: Dan Gora <***@adax.com>
---
ipmitool/include/ipmitool/ipmi_sdr.h | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ipmitool/include/ipmitool/ipmi_sdr.h b/ipmitool/include/ipmitool/ipmi_sdr.h
index 3c88397..cd7742f 100644
--- a/ipmitool/include/ipmitool/ipmi_sdr.h
+++ b/ipmitool/include/ipmitool/ipmi_sdr.h
@@ -809,15 +809,15 @@ static const char *sensor_type_desc[] __attribute__ ((unused)) = {
"Physical Security", "Platform Security", "Processor",
"Power Supply", "Power Unit", "Cooling Device", "Other",
"Memory", "Drive Slot / Bay", "POST Memory Resize",
- "System Firmwares", "Event Logging Disabled", "Watchdog",
+ "System Firmwares", "Event Logging Disabled", "Watchdog1",
"System Event", "Critical Interrupt", "Button",
"Module / Board", "Microcontroller", "Add-in Card",
"Chassis", "Chip Set", "Other FRU", "Cable / Interconnect",
"Terminator", "System Boot Initiated", "Boot Error",
"OS Boot", "OS Critical Stop", "Slot / Connector",
- "System ACPI Power State", "Watchdog", "Platform Alert",
+ "System ACPI Power State", "Watchdog2", "Platform Alert",
"Entity Presence", "Monitor ASIC", "LAN",
- "Management Subsystem Health", "Battery","Session Audit",
+ "Management Subsys Health", "Battery","Session Audit",
"Version Change","FRU State" };

struct sensor_reading {
--
1.7.7
Zdenek Styblik
2013-04-01 18:54:10 UTC
Permalink
* diff touches more than just help
* WRONG >> printf("Sensor Type %s not found.\n", type);
Post by Dan Gora
Fixed the formatting of the help command to make it clearer what
options the sdr command has.
Dan,

here is what I see wrong with this patch:

* it touches/modifies more than just help
* ``printf("Sensor Type %s not found.\n", type);'' -> ``lprintf(LOG_ERR, ...);''

I find the following, despite not covered/modified by the patch, to be
disputable as well: ``printf("Sensor Types:\n");''. I think this
should go to ``lprintf(LOG_NOTICE, ...);''. Just a note/observation.

Best regards,
Z.
Post by Dan Gora
---
ipmitool/lib/ipmi_sdr.c | 71 +++++++++++++++++++++++++++++------------------
1 files changed, 44 insertions(+), 27 deletions(-)
diff --git a/ipmitool/lib/ipmi_sdr.c b/ipmitool/lib/ipmi_sdr.c
index 698f051..272dee0 100644
--- a/ipmitool/lib/ipmi_sdr.c
+++ b/ipmitool/lib/ipmi_sdr.c
@@ -3743,7 +3743,7 @@ ipmi_sdr_find_sdr_byid(struct ipmi_intf *intf, char *id)
}
/* now keep looking */
- while ((header = ipmi_sdr_get_next_header(intf, sdr_list_itr)) != NULL) {
+ while ((header = ipmi_sdr_get_next_header(intf, sdr_list_itr)) != NULL){
uint8_t *rec;
struct sdr_record_list *sdrr;
@@ -4406,6 +4406,7 @@ ipmi_sdr_print_type(struct ipmi_intf *intf, char *type)
}
}
if (sensor_type != x) {
+ printf("Sensor Type %s not found.\n", type);
printf("Sensor Types:\n");
for (x = 1; x < SENSOR_TYPE_MAX; x += 2) {
printf("\t%-25s %-25s\n",
@@ -4552,49 +4553,65 @@ ipmi_sdr_main(struct ipmi_intf *intf, int argc, char **argv)
if (argc == 0)
return ipmi_sdr_print_sdr(intf, 0xfe);
else if (strncmp(argv[0], "help", 4) == 0) {
+ lprintf(LOG_ERR, "SDR Commands:");
lprintf(LOG_ERR,
- "SDR Commands: list | elist [all|full|compact|event|mcloc|fru|generic]");
+ " list | elist [option]. Available options are:");
lprintf(LOG_ERR,
- " all All SDR Records");
+ " all All SDR Records");
lprintf(LOG_ERR,
- " full Full Sensor Record");
+ " full Full Sensor Record");
lprintf(LOG_ERR,
- " compact Compact Sensor Record");
+ " compact Compact Sensor Record");
lprintf(LOG_ERR,
- " event Event-Only Sensor Record");
+ " event Event-Only Sensor Record");
lprintf(LOG_ERR,
- " mcloc Management Controller Locator Record");
+ " mcloc Management Controller Locator Record");
lprintf(LOG_ERR,
- " fru FRU Locator Record");
+ " fru FRU Locator Record");
lprintf(LOG_ERR,
- " generic Generic Device Locator Record");
- lprintf(LOG_ERR, " type [sensor type]");
+ " generic Generic Device Locator Record");
+ lprintf(LOG_ERR,
+ " type [sensor type] | list");
lprintf(LOG_ERR,
- " list Get a list of available sensor types");
+ " Retrieve sensors matching [sensor type]");
lprintf(LOG_ERR,
- " get Retrieve the state of a specified sensor");
-
- lprintf(LOG_ERR, " info");
+ " list Get a list of available sensor types");
lprintf(LOG_ERR,
- " Display information about the repository itself");
- lprintf(LOG_ERR, " entity <id>[.<instance>]");
+ " get");
lprintf(LOG_ERR,
- " Display all sensors associated with an entity");
- lprintf(LOG_ERR, " dump <file>");
+ " Retrieve the state of a specified sensor");
+ lprintf(LOG_ERR,
+ " info");
lprintf(LOG_ERR,
- " Dump raw SDR data to a file");
- lprintf(LOG_ERR, " fill");
+ " Display information about the repository itself");
+ lprintf(LOG_ERR,
+ " entity <id>[.<instance>]");
lprintf(LOG_ERR,
- " sensors Creates the SDR repository for the current configuration");
+ " Display all sensors associated with an entity");
+ lprintf(LOG_ERR,
+ " dump <file>");
lprintf(LOG_ERR,
- " nosat Creates the SDR repository for the current configuration, without satellite scan");
-
+ " Dump raw SDR data to a file");
+ lprintf(LOG_ERR,
+ " fill [option]. Available options:");
+ lprintf(LOG_ERR,
+ " sensors Creates the SDR repository for the current");
+ lprintf(LOG_ERR,
+ " configuration.");
+ lprintf(LOG_ERR,
+ " nosat Creates the SDR repository for the current");
+ lprintf(LOG_ERR,
+ " configuration, without satellite scan.");
+ lprintf(LOG_ERR,
+ " file <file> Load SDR repository from a file");
+ lprintf(LOG_ERR,
+ " range <range> Load SDR repository from a provided list");
lprintf(LOG_ERR,
- " file <file> Load SDR repository from a file");
+ " or range");
lprintf(LOG_ERR,
- " range <range> Load SDR repository from a provided list or range");
+ " Use , for list or - for range");
lprintf(LOG_ERR,
- " - Use , for list or - for range (Ex.: 0x28,0x32,0x40-0x44) ");
+ " (Ex.: 0x28,0x32,0x40-0x44) ");
} else if (strncmp(argv[0], "list", 4) == 0
|| strncmp(argv[0], "elist", 5) == 0) {
@@ -4661,7 +4678,7 @@ ipmi_sdr_main(struct ipmi_intf *intf, int argc, char **argv)
rc = -1;
} else if (strncmp(argv[1], "sensors", 7) == 0) {
rc = ipmi_sdr_add_from_sensors(intf, 21);
- } else if (strncmp(argv[1], "nosats", 6) == 0) {
+ } else if (strncmp(argv[1], "nosat", 5) == 0) {
rc = ipmi_sdr_add_from_sensors(intf, 0);
} else if (strncmp(argv[1], "file", 4) == 0) {
if (argc < 3) {
--
1.7.7
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
Dan Gora
2013-04-01 19:03:31 UTC
Permalink
Post by Zdenek Styblik
Dan,
* it touches/modifies more than just help
* ``printf("Sensor Type %s not found.\n", type);'' -> ``lprintf(LOG_ERR, ...);''
I find the following, despite not covered/modified by the patch, to be
disputable as well: ``printf("Sensor Types:\n");''. I think this
should go to ``lprintf(LOG_NOTICE, ...);''. Just a note/observation.
So just change these:
if (sensor_type != x) {
+ printf("Sensor Type %s not found.\n", type);
printf("Sensor Types:\n");

to use lprintf(LOG_NOTICE, ..)?

Ok, no problem.. I'll fix this.

thanks
dan
Zdenek Styblik
2013-04-01 19:18:55 UTC
Permalink
Post by Dan Gora
Post by Zdenek Styblik
Dan,
* it touches/modifies more than just help
* ``printf("Sensor Type %s not found.\n", type);'' -> ``lprintf(LOG_ERR, ...);''
I find the following, despite not covered/modified by the patch, to be
disputable as well: ``printf("Sensor Types:\n");''. I think this
should go to ``lprintf(LOG_NOTICE, ...);''. Just a note/observation.
if (sensor_type != x) {
+ printf("Sensor Type %s not found.\n", type);
printf("Sensor Types:\n");
to use lprintf(LOG_NOTICE, ..)?
Ok, no problem.. I'll fix this.
thanks
dan
lprintf(LOG_ERR, "Sensor Type %s not found.", type);

I presume this is "help" context, but I definitely should check it in
application context:
lprintf(LOG_NOTICE, "Sensor Types:");
for (x = 1; x < SENSOR_TYPE_MAX; x += 2) {
lprintf(LOG_NOTICE, "\t%-25s %-25s",
sensor_type_desc[x], sensor_type_desc[x + 1]);
}

Funny enough, I see a lot of help output going on LOG_ERR in
'lib/ipmi_sdr.c' instead of LOG_NOTICE. *sigh*

Regards,
Z.
Dan Gora
2013-04-01 19:22:16 UTC
Permalink
Post by Zdenek Styblik
lprintf(LOG_ERR, "Sensor Type %s not found.", type);
I presume this is "help" context, but I definitely should check it in
lprintf(LOG_NOTICE, "Sensor Types:");
for (x = 1; x < SENSOR_TYPE_MAX; x += 2) {
lprintf(LOG_NOTICE, "\t%-25s %-25s",
sensor_type_desc[x], sensor_type_desc[x + 1]);
}
Funny enough, I see a lot of help output going on LOG_ERR in
'lib/ipmi_sdr.c' instead of LOG_NOTICE. *sigh*
Ok, I'll take a look at this and see if I can't clean it up a bit.

thanks
d
Zdenek Styblik
2013-04-04 12:48:14 UTC
Permalink
Post by Dan Gora
Fixed the formatting of the help command to make it clearer what
options the sdr command has.
Ok, I did take a closer look at this diff:
1st part - useless and won't be committed
2nd part - we've talked about it already, it will be changed to
lprintf(LOG_ERR, ...); ok to commit after the change
3rd part - sorry, but NACK.
4th part - ok to commit

Each part should be committed separately(as a small commit), but can
be tracked under the same tracker ID.

Z.
Post by Dan Gora
---
ipmitool/lib/ipmi_sdr.c | 71 +++++++++++++++++++++++++++++------------------
1 files changed, 44 insertions(+), 27 deletions(-)
diff --git a/ipmitool/lib/ipmi_sdr.c b/ipmitool/lib/ipmi_sdr.c
index 698f051..272dee0 100644
--- a/ipmitool/lib/ipmi_sdr.c
+++ b/ipmitool/lib/ipmi_sdr.c
@@ -3743,7 +3743,7 @@ ipmi_sdr_find_sdr_byid(struct ipmi_intf *intf, char *id)
}
/* now keep looking */
- while ((header = ipmi_sdr_get_next_header(intf, sdr_list_itr)) != NULL) {
+ while ((header = ipmi_sdr_get_next_header(intf, sdr_list_itr)) != NULL){
uint8_t *rec;
struct sdr_record_list *sdrr;
@@ -4406,6 +4406,7 @@ ipmi_sdr_print_type(struct ipmi_intf *intf, char *type)
}
}
if (sensor_type != x) {
+ printf("Sensor Type %s not found.\n", type);
printf("Sensor Types:\n");
for (x = 1; x < SENSOR_TYPE_MAX; x += 2) {
printf("\t%-25s %-25s\n",
@@ -4552,49 +4553,65 @@ ipmi_sdr_main(struct ipmi_intf *intf, int argc, char **argv)
if (argc == 0)
return ipmi_sdr_print_sdr(intf, 0xfe);
else if (strncmp(argv[0], "help", 4) == 0) {
+ lprintf(LOG_ERR, "SDR Commands:");
lprintf(LOG_ERR,
- "SDR Commands: list | elist [all|full|compact|event|mcloc|fru|generic]");
+ " list | elist [option]. Available options are:");
lprintf(LOG_ERR,
- " all All SDR Records");
+ " all All SDR Records");
lprintf(LOG_ERR,
- " full Full Sensor Record");
+ " full Full Sensor Record");
lprintf(LOG_ERR,
- " compact Compact Sensor Record");
+ " compact Compact Sensor Record");
lprintf(LOG_ERR,
- " event Event-Only Sensor Record");
+ " event Event-Only Sensor Record");
lprintf(LOG_ERR,
- " mcloc Management Controller Locator Record");
+ " mcloc Management Controller Locator Record");
lprintf(LOG_ERR,
- " fru FRU Locator Record");
+ " fru FRU Locator Record");
lprintf(LOG_ERR,
- " generic Generic Device Locator Record");
- lprintf(LOG_ERR, " type [sensor type]");
+ " generic Generic Device Locator Record");
+ lprintf(LOG_ERR,
+ " type [sensor type] | list");
lprintf(LOG_ERR,
- " list Get a list of available sensor types");
+ " Retrieve sensors matching [sensor type]");
lprintf(LOG_ERR,
- " get Retrieve the state of a specified sensor");
-
- lprintf(LOG_ERR, " info");
+ " list Get a list of available sensor types");
lprintf(LOG_ERR,
- " Display information about the repository itself");
- lprintf(LOG_ERR, " entity <id>[.<instance>]");
+ " get");
lprintf(LOG_ERR,
- " Display all sensors associated with an entity");
- lprintf(LOG_ERR, " dump <file>");
+ " Retrieve the state of a specified sensor");
+ lprintf(LOG_ERR,
+ " info");
lprintf(LOG_ERR,
- " Dump raw SDR data to a file");
- lprintf(LOG_ERR, " fill");
+ " Display information about the repository itself");
+ lprintf(LOG_ERR,
+ " entity <id>[.<instance>]");
lprintf(LOG_ERR,
- " sensors Creates the SDR repository for the current configuration");
+ " Display all sensors associated with an entity");
+ lprintf(LOG_ERR,
+ " dump <file>");
lprintf(LOG_ERR,
- " nosat Creates the SDR repository for the current configuration, without satellite scan");
-
+ " Dump raw SDR data to a file");
+ lprintf(LOG_ERR,
+ " fill [option]. Available options:");
+ lprintf(LOG_ERR,
+ " sensors Creates the SDR repository for the current");
+ lprintf(LOG_ERR,
+ " configuration.");
+ lprintf(LOG_ERR,
+ " nosat Creates the SDR repository for the current");
+ lprintf(LOG_ERR,
+ " configuration, without satellite scan.");
+ lprintf(LOG_ERR,
+ " file <file> Load SDR repository from a file");
+ lprintf(LOG_ERR,
+ " range <range> Load SDR repository from a provided list");
lprintf(LOG_ERR,
- " file <file> Load SDR repository from a file");
+ " or range");
lprintf(LOG_ERR,
- " range <range> Load SDR repository from a provided list or range");
+ " Use , for list or - for range");
lprintf(LOG_ERR,
- " - Use , for list or - for range (Ex.: 0x28,0x32,0x40-0x44) ");
+ " (Ex.: 0x28,0x32,0x40-0x44) ");
} else if (strncmp(argv[0], "list", 4) == 0
|| strncmp(argv[0], "elist", 5) == 0) {
@@ -4661,7 +4678,7 @@ ipmi_sdr_main(struct ipmi_intf *intf, int argc, char **argv)
rc = -1;
} else if (strncmp(argv[1], "sensors", 7) == 0) {
rc = ipmi_sdr_add_from_sensors(intf, 21);
- } else if (strncmp(argv[1], "nosats", 6) == 0) {
+ } else if (strncmp(argv[1], "nosat", 5) == 0) {
rc = ipmi_sdr_add_from_sensors(intf, 0);
} else if (strncmp(argv[1], "file", 4) == 0) {
if (argc < 3) {
--
1.7.7
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
Dan Gora
2013-04-04 16:52:38 UTC
Permalink
Post by Zdenek Styblik
Post by Dan Gora
Fixed the formatting of the help command to make it clearer what
options the sdr command has.
1st part - useless and won't be committed
2nd part - we've talked about it already, it will be changed to
lprintf(LOG_ERR, ...); ok to commit after the change
3rd part - sorry, but NACK.
4th part - ok to commit
Ok, I really have no idea what "parts" you are referring to here.
Also, can you give a reason for NACKing whatever it is that you are
NACKing?

thanks
dan
Dan Gora
2013-04-04 17:28:46 UTC
Permalink
This is what this patch is fixing. Here is what 'sdr help' looks like
before the changes:

dg:speedy:build(master) => src/ipmitool sdr help
Could not open device at /dev/ipmi0 or /dev/ipmi/0 or /dev/ipmidev/0:
No such file or directory
SDR Commands: list | elist [all|full|compact|event|mcloc|fru|generic]
all All SDR Records
full Full Sensor Record
compact Compact Sensor Record
event Event-Only Sensor Record
mcloc Management Controller Locator Record
fru FRU Locator Record
generic Generic Device Locator Record
type [sensor type]
list Get a list of available sensor types
get Retrieve the state of a specified sensor
info
Display information about the repository itself
entity <id>[.<instance>]
Display all sensors associated with an entity
dump <file>
Dump raw SDR data to a file
fill
sensors Creates the SDR repository for the
current configuration
nosat Creates the SDR repository for the
current configuration, without satellite scan
file <file> Load SDR repository from a file
range <range> Load SDR repository from a provided
list or range
- Use , for list or - for range
(Ex.: 0x28,0x32,0x40-0x44)


This is what it looks like after the changes:

dg:speedy:build(master) => src/ipmitool sdr help
Could not open device at /dev/ipmi0 or /dev/ipmi/0 or /dev/ipmidev/0:
No such file or directory
SDR Commands:
list | elist [option]. Available options are:
all All SDR Records
full Full Sensor Record
compact Compact Sensor Record
event Event-Only Sensor Record
mcloc Management Controller Locator Record
fru FRU Locator Record
generic Generic Device Locator Record
type [sensor type] | list
Retrieve sensors matching [sensor type].
[sensor type] can be specified as hex value or a string.
list Get a list of available sensor types
get [Sensor ID string]
Retrieve the first sensor with Sensor ID matching
[Sensor ID string].
info
Display information about the repository itself
entity <id>[.<instance>]
Display all sensors associated with an entity
dump <file>
Dump raw SDR data to a file
fill [option]. Available options:
sensors Creates the SDR repository for the current
configuration.
nosat Creates the SDR repository for the current
configuration, without satellite scan.
file <file> Load SDR repository from a file
range <range> Load SDR repository from a provided list
or range
Use , for list or - for range
(Ex.: 0x28,0x32,0x40-0x44)

Note that long lines are broken up so that things don't go past 80
columns and the help has been clarified and updated. It's not just
messing with the formatting of the code.

thanks
dan
Post by Dan Gora
Post by Zdenek Styblik
Post by Dan Gora
Fixed the formatting of the help command to make it clearer what
options the sdr command has.
1st part - useless and won't be committed
2nd part - we've talked about it already, it will be changed to
lprintf(LOG_ERR, ...); ok to commit after the change
3rd part - sorry, but NACK.
4th part - ok to commit
Ok, I really have no idea what "parts" you are referring to here.
Also, can you give a reason for NACKing whatever it is that you are
NACKing?
thanks
dan
--
Dan Gora
Software Engineer

Adax, Inc.
Av Dona Maria Alves, 1070 Casa 5
Centro
Ubatuba, SP
CEP 11680-000
Brasil

Tel: +55 (12) 3833-1021 (Brazil and outside of US)
: +1 (510) 859-4801 (Inside of US)
: dan_gora (Skype)

email: ***@adax.com
Zdenek Styblik
2013-04-04 19:52:22 UTC
Permalink
Post by Dan Gora
Post by Zdenek Styblik
Post by Dan Gora
Fixed the formatting of the help command to make it clearer what
options the sdr command has.
1st part - useless and won't be committed
2nd part - we've talked about it already, it will be changed to
lprintf(LOG_ERR, ...); ok to commit after the change
3rd part - sorry, but NACK.
4th part - ok to commit
Ok, I really have no idea what "parts" you are referring to here.
Also, can you give a reason for NACKing whatever it is that you are
NACKing?
thanks
dan
Sure. I've found two issues with help "update". The first is, I don't
like new help output very much, to be honest. That doesn't mean you
should re-work it. Let me come up with my version.
The second issue I have is formatting. I understand you want to
improve code formatting and I'm all for that. But replacing tabs with
(one) white space and/or removing white spaces etc.? I really don't
see it as a way to go.

~~~
<this is tabbed>lprintf(LOG_ERR,
"whoo, where does this belong?"
<this is tabbed>whatever_is_here
~~~

I don't find it very nice, no.
If anything, string passed to printf/lprintf can be split. Does it
make sense for something like sentences or some long text? I'm not
sure. And if I remember correctly, even GNU coding standards, which
are considered ancient and obsolete nowadays, allow lines over 80
chars if there is no (intelligent?) way to split it.

*shrug*

Regards,
Z.
Dan Gora
2013-04-04 19:57:58 UTC
Permalink
Post by Zdenek Styblik
Sure. I've found two issues with help "update". The first is, I don't
like new help output very much, to be honest. That doesn't mean you
Well it would help if you could actually say what you don't like about
it. I think that it's much clearer and actually fixes problems with
the help output from before.
Post by Zdenek Styblik
should re-work it. Let me come up with my version.
The second issue I have is formatting. I understand you want to
improve code formatting and I'm all for that. But replacing tabs with
(one) white space and/or removing white spaces etc.? I really don't
see it as a way to go.
~~~
<this is tabbed>lprintf(LOG_ERR,
"whoo, where does this belong?"
<this is tabbed>whatever_is_here
~~~
I don't find it very nice, no.
This is like this for a very important reason.

By starting all of the strings on a new line at the same offset, it
becomes much, much easier to actually format the text inside the
quotes. You can see in your text editor exactly how everything will
line up. By placing them on the same line, not only does the line
wrap, but you have to actually run the program to see how the text is
going to display on the screen.
Post by Zdenek Styblik
If anything, string passed to printf/lprintf can be split. Does it
make sense for something like sentences or some long text? I'm not
sure. And if I remember correctly, even GNU coding standards, which
are considered ancient and obsolete nowadays, allow lines over 80
chars if there is no (intelligent?) way to split it.
The whole point here is that you don't _want_ the strings to be longer
than 80 chars. By placing them on their own line you can see
_without_ having to run the program that the line will not wrap when
it is run.

thanks
dan
Zdenek Styblik
2013-04-04 20:23:31 UTC
Permalink
Post by Dan Gora
Post by Zdenek Styblik
Sure. I've found two issues with help "update". The first is, I don't
like new help output very much, to be honest. That doesn't mean you
Well it would help if you could actually say what you don't like about
it. I think that it's much clearer and actually fixes problems with
the help output from before.
Post by Zdenek Styblik
should re-work it. Let me come up with my version.
The second issue I have is formatting. I understand you want to
improve code formatting and I'm all for that. But replacing tabs with
(one) white space and/or removing white spaces etc.? I really don't
see it as a way to go.
~~~
<this is tabbed>lprintf(LOG_ERR,
"whoo, where does this belong?"
<this is tabbed>whatever_is_here
~~~
I don't find it very nice, no.
This is like this for a very important reason.
By starting all of the strings on a new line at the same offset, it
becomes much, much easier to actually format the text inside the
quotes. You can see in your text editor exactly how everything will
line up. By placing them on the same line, not only does the line
wrap, but you have to actually run the program to see how the text is
going to display on the screen.
Post by Zdenek Styblik
If anything, string passed to printf/lprintf can be split. Does it
make sense for something like sentences or some long text? I'm not
sure. And if I remember correctly, even GNU coding standards, which
are considered ancient and obsolete nowadays, allow lines over 80
chars if there is no (intelligent?) way to split it.
The whole point here is that you don't _want_ the strings to be longer
than 80 chars. By placing them on their own line you can see
_without_ having to run the program that the line will not wrap when
it is run.
thanks
dan
Really *shrug*. (now, you knew it had to come some time) Sometimes I
even ask myself whether 80 chars width is still relevant nowadays.
Relevant for majority, that is. And I don't mean only code formatting
now, but output as well. Oh, and I really mean it.
Anyway, what you've written makes sense, sort of. But don't do such
thing in the middle of the code. Say, if this is done in function
which does nothing else just these print-outs, that's fine. I don't
think it's fine doing it in some nested code(not 100% relevant to
'ipmi_sdr.c' as the large help print-out is right at the beginning).
In the end, it will be better to shove that help text into it's own function.

Z.
Dan Gora
2013-04-04 20:32:27 UTC
Permalink
Post by Zdenek Styblik
Really *shrug*. (now, you knew it had to come some time) Sometimes I
even ask myself whether 80 chars width is still relevant nowadays.
yes! It is!
Post by Zdenek Styblik
Relevant for majority, that is. And I don't mean only code formatting
now, but output as well. Oh, and I really mean it.
You might not like it, but that's the standard for pretty much everything.

There still exist many terminals which only support 80 columns of output.
Post by Zdenek Styblik
Anyway, what you've written makes sense, sort of. But don't do such
thing in the middle of the code. Say, if this is done in function
which does nothing else just these print-outs, that's fine. I don't
think it's fine doing it in some nested code(not 100% relevant to
'ipmi_sdr.c' as the large help print-out is right at the beginning).
In the end, it will be better to shove that help text into it's own function.
That's fine to put it in it's own function, but it wasn't before...
I mean really, I just find it really hard to believe that you'd think
that the way it was before was more readable.

thanks
dan
Zdenek Styblik
2013-04-09 07:31:02 UTC
Permalink
Post by Dan Gora
Fixed the formatting of the help command to make it clearer what
options the sdr command has.
Hello,

attached is diff and output of my take on 'sdr help'. Diff
incorporates all proposed changes to 'sdr help'. Originally, changes
were split into two diffs. To be honest, it's not that much different
from the original, but there are differences.
I particularly didn't like this ``"list | elist [option]. Available
options are:");'', because I think it's pointless and if not
pointless, then ugly to put it inline. Another difference is 'sdr get'
actually requires Sensor_ID to be supplied, so '<...>' has to be used
instead of '[...]'. There are some other changes, see the diff.
Since I'm not a native English speaker, it makes sense if somebody who
is checks spelling etc.

Best regards,
Z.
Ales Ledvinka
2013-04-10 08:37:35 UTC
Permalink
ack Z.

----- Original Message -----
From: "Zdenek Styblik" <***@gmail.com>
To: "Dan Gora" <***@adax.com>
Cc: "ipmitool-devel" <ipmitool-***@lists.sourceforge.net>
Sent: Tuesday, April 9, 2013 9:31:02 AM
Subject: Re: [Ipmitool-devel] [PATCH 1/3] Fixed help for sdr command.
Post by Dan Gora
Fixed the formatting of the help command to make it clearer what
options the sdr command has.
Hello,

attached is diff and output of my take on 'sdr help'. Diff
incorporates all proposed changes to 'sdr help'. Originally, changes
were split into two diffs. To be honest, it's not that much different
from the original, but there are differences.
I particularly didn't like this ``"list | elist [option]. Available
options are:");'', because I think it's pointless and if not
pointless, then ugly to put it inline. Another difference is 'sdr get'
actually requires Sensor_ID to be supplied, so '<...>' has to be used
instead of '[...]'. There are some other changes, see the diff.
Since I'm not a native English speaker, it makes sense if somebody who
is checks spelling etc.

Best regards,
Z.
Zdenek Styblik
2013-04-12 12:55:04 UTC
Permalink
Hi all,

attached is one more, and hopefully last, iteration on SDR help
update. The most notable changes from previous version:
* LOG_ERR -> LOG_NOTICE
* added error messages ``Not enough parameters given.''
* removed help text about how to define/input range in "small" help
print-out as it looked awful and out of place
* added handling of 'sdr fill INVALID_SUBCMD' case

Have a nice weekend,
Z.

--
Zdenek Styblik
Post by Ales Ledvinka
ack Z.
----- Original Message -----
Sent: Tuesday, April 9, 2013 9:31:02 AM
Subject: Re: [Ipmitool-devel] [PATCH 1/3] Fixed help for sdr command.
Post by Dan Gora
Fixed the formatting of the help command to make it clearer what
options the sdr command has.
Hello,
attached is diff and output of my take on 'sdr help'. Diff
incorporates all proposed changes to 'sdr help'. Originally, changes
were split into two diffs. To be honest, it's not that much different
from the original, but there are differences.
I particularly didn't like this ``"list | elist [option]. Available
options are:");'', because I think it's pointless and if not
pointless, then ugly to put it inline. Another difference is 'sdr get'
actually requires Sensor_ID to be supplied, so '<...>' has to be used
instead of '[...]'. There are some other changes, see the diff.
Since I'm not a native English speaker, it makes sense if somebody who
is checks spelling etc.
Best regards,
Z.
------------------------------------------------------------------------------
Precog is a next-generation analytics platform capable of advanced
analytics on semi-structured data. The platform includes APIs for building
apps and a phenomenal toolset for data science. Developers can use
our toolset for easy data analysis & visualization. Get a free account!
http://www2.precog.com/precogplatform/slashdotnewsletter
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
Loading...