On Mon, Mar 25, 2013 at 10:16 AM, Zdenek Styblik
<zdenek.styb...@gmail.com> wrote:
> On Fri, Mar 22, 2013 at 2:40 PM, Corey Minyard <tcminy...@gmail.com> wrote:
>> The SOL protocol supports multiple serial ports using the "instance",
>> allow this to be passed in to ipmitool.
>>
>
> Corey,
>
> please, use str2*() functions from 'lib/helper.c' rather than
> strtol(). Unless this is fixed, I'm against getting this one into CVS.
> Is it really necessary to use 'unsigned int' for 'instance' since its
> range is only <1..15>, resp. <1..255>? Wouldn't it be sufficient to
> use 'uint8_t'?
> Would it be possible to use int types from 'stdint.h'?
>
> ~~~
> - retval = ipmi_sol_activate(intf, 1, interval);
> + retval = ipmi_sol_activate(intf, 1, interval, 1);
> ~~~
> Hard-coded instance number to '1'; is this on purpose(just checking/asking)?
>
> Best regards,
> Z.
>
One more thing. I think this check ``if (instance < 1 || instance >
14) { /* Error */ }'' should be moved into ipmi_sol_activate(), resp.
ipmi_sol_deactivate() or perhaps even create new function(wrapper) for
it.
Z.
>
>> Index: doc/ipmitool.1
>> ===================================================================
>> RCS file: /cvsroot/ipmitool/ipmitool/doc/ipmitool.1,v
>> retrieving revision 1.50
>> diff -u -r1.50 ipmitool.1
>> --- doc/ipmitool.1 5 Sep 2012 14:22:36 -0000 1.50
>> +++ doc/ipmitool.1 22 Mar 2013 13:33:41 -0000
>> @@ -2586,7 +2586,7 @@
>> by the IPMI over serial channel.
>> .RE
>> .TP
>> -\fIactivate\fP [\fIusesolkeepalive\fP | \fInokeepalive\fP]
>> +\fIactivate\fP [\fIusesolkeepalive\fP | \fInokeepalive\fP]
>> [\fIinstance=<number>\fP]
>> .br
>>
>> Causes ipmitool to enter Serial Over LAN
>> @@ -2596,6 +2596,9 @@
>> sent to the serial console on the remote server.
>> On exit,the the SOL payload mode is deactivated and
>> the terminal is reset to its original settings.
>> +
>> +If the instance is given, it will activate using the given instance
>> +number. The default is 1.
>> .RS
>>
>> Special escape sequences are provided to control the SOL session:
>> @@ -2617,7 +2620,7 @@
>> Note that escapes are only recognized immediately after newline.
>> .RE
>> .TP
>> -\fIdeactivate\fP
>> +\fIdeactivate\fP [\fIinstance=<number>\fP]
>> .br
>>
>> Deactivates Serial Over LAN mode on the BMC.
>> @@ -2625,6 +2628,9 @@
>> this command to be sent to the BMC, but in the case of an
>> unintentional exit from SOL mode, this command may be
>> necessary to reset the state of the BMC.
>> +
>> +If the instance is given, it will deactivate the given instance
>> +number. The default is 1.
>> .RE
>> .TP
>> \fIspd\fP <\fBi2cbus\fR> <\fBi2caddr\fR> [<\fBchannel\fR>]
>> [<\fmaxread\fR>]
>> Index: lib/ipmi_sol.c
>> ===================================================================
>> RCS file: /cvsroot/ipmitool/ipmitool/lib/ipmi_sol.c,v
>> retrieving revision 1.62
>> diff -u -r1.62 ipmi_sol.c
>> --- lib/ipmi_sol.c 18 Jan 2013 12:37:27 -0000 1.62
>> +++ lib/ipmi_sol.c 22 Mar 2013 13:33:42 -0000
>> @@ -1292,7 +1292,7 @@
>> * ipmi_sol_deactivate
>> */
>> static int
>> -ipmi_sol_deactivate(struct ipmi_intf * intf)
>> +ipmi_sol_deactivate(struct ipmi_intf * intf, int instance)
>> {
>> struct ipmi_rs * rsp;
>> struct ipmi_rq req;
>> @@ -1305,8 +1305,8 @@
>> req.msg.data = data;
>>
>> bzero(data, sizeof(data));
>> - data[0] = IPMI_PAYLOAD_TYPE_SOL; /* payload type */
>> - data[1] = 1; /* payload instance. Guess! */
>> + data[0] = IPMI_PAYLOAD_TYPE_SOL; /* payload type */
>> + data[1] = instance; /* payload instance. */
>>
>> /* Lots of important data */
>> data[2] = 0;
>> @@ -1533,7 +1533,7 @@
>> * ipmi_sol_red_pill
>> */
>> static int
>> -ipmi_sol_red_pill(struct ipmi_intf * intf)
>> +ipmi_sol_red_pill(struct ipmi_intf * intf, int instance)
>> {
>> char * buffer;
>> int numRead;
>> @@ -1679,7 +1679,7 @@
>> {
>> lprintf(LOG_ERR, "Error: No response to keepalive -
>> Terminating session");
>> /* attempt to clean up anyway */
>> - ipmi_sol_deactivate(intf);
>> + ipmi_sol_deactivate(intf, instance);
>> exit(1);
>> }
>>
>> @@ -1689,7 +1689,7 @@
>> exit(1);
>> }
>> else
>> - ipmi_sol_deactivate(intf);
>> + ipmi_sol_deactivate(intf, instance);
>>
>> return 0;
>> }
>> @@ -1701,7 +1701,8 @@
>> * ipmi_sol_activate
>> */
>> static int
>> -ipmi_sol_activate(struct ipmi_intf * intf, int looptest, int interval)
>> +ipmi_sol_activate(struct ipmi_intf * intf, int looptest, int interval,
>> + int instance)
>> {
>> struct ipmi_rs * rsp;
>> struct ipmi_rq req;
>> @@ -1737,7 +1738,7 @@
>> req.msg.data = data;
>>
>> data[0] = IPMI_PAYLOAD_TYPE_SOL; /* payload type */
>> - data[1] = 1; /* payload instance */
>> + data[1] = instance; /* payload instance */
>>
>> /* Lots of important data. Most is default */
>> data[2] = bSolEncryption? 0x80 : 0;
>> @@ -1843,7 +1844,7 @@
>>
>> if(looptest == 1)
>> {
>> - ipmi_sol_deactivate(intf);
>> + ipmi_sol_deactivate(intf, instance);
>> usleep(interval*1000);
>> return 0;
>> }
>> @@ -1854,7 +1855,7 @@
>> * 1) STDIN for user input
>> * 2) The FD for incoming SOL packets
>> */
>> - if (ipmi_sol_red_pill(intf))
>> + if (ipmi_sol_red_pill(intf, instance))
>> {
>> lprintf(LOG_ERR, "Error in SOL session");
>> return -1;
>> @@ -1874,8 +1875,8 @@
>> lprintf(LOG_NOTICE, "SOL Commands: info [<channel number>]");
>> lprintf(LOG_NOTICE, " set <parameter> <value>
>> [channel]");
>> lprintf(LOG_NOTICE, " payload <enable|disable|status>
>> [channel] [userid]");
>> - lprintf(LOG_NOTICE, " activate
>> [<usesolkeepalive|nokeepalive>]");
>> - lprintf(LOG_NOTICE, " deactivate");
>> + lprintf(LOG_NOTICE, " activate
>> [<usesolkeepalive|nokeepalive>] [instance=<number>]");
>> + lprintf(LOG_NOTICE, " deactivate [instance=<number>]");
>> lprintf(LOG_NOTICE, " looptest [<loop times>] [<loop
>> interval(in ms)>]");
>> }
>>
>> @@ -2029,32 +2030,60 @@
>> * Activate
>> */
>> else if (!strncmp(argv[0], "activate", 8)) {
>> + int i;
>> + unsigned int instance = 1;
>>
>> - if (argc > 2) {
>> - print_sol_usage();
>> - return -1;
>> - }
>> -
>> - if (argc == 2) {
>> - if (!strncmp(argv[1], "usesolkeepalive", 15))
>> + for (i = 1; i < argc; i++) {
>> + if (!strncmp(argv[i], "usesolkeepalive", 15))
>> _use_sol_for_keepalive = 1;
>> - else if (!strncmp(argv[1], "nokeepalive", 11))
>> + else if (!strncmp(argv[i], "nokeepalive", 11))
>> _disable_keepalive = 1;
>> - else {
>> + else if (!strncmp(argv[i], "instance=", 9)) {
>> + char *end;
>> + instance = strtoul(argv[i] + 9, &end, 0);
>> + if (*end != 0 || instance == 0 || instance > 15)
>> + {
>> + lprintf(LOG_ERR,
>> + "instance invalid: '%s'",
>> + argv[i] + 9);
>> + print_sol_usage();
>> + return -1;
>> + }
>> + } else {
>> print_sol_usage();
>> return -1;
>> }
>> }
>> - retval = ipmi_sol_activate(intf, 0, 0);
>> + retval = ipmi_sol_activate(intf, 0, 0, instance);
>> }
>>
>>
>> /*
>> * Dectivate
>> */
>> - else if (!strncmp(argv[0], "deactivate", 10))
>> - retval = ipmi_sol_deactivate(intf);
>> -
>> + else if (!strncmp(argv[0], "deactivate", 10)) {
>> + int i;
>> + unsigned int instance = 1;
>> +
>> + for (i = 1; i < argc; i++) {
>> + if (!strncmp(argv[i], "instance=", 9)) {
>> + char *end;
>> + instance = strtoul(argv[i] + 9, &end, 0);
>> + if (*end != 0 || instance == 0 || instance > 15)
>> + {
>> + lprintf(LOG_ERR,
>> + "instance invalid: '%s'",
>> + argv[i] + 9);
>> + print_sol_usage();
>> + return -1;
>> + }
>> + } else {
>> + print_sol_usage();
>> + return -1;
>> + }
>> + }
>> + retval = ipmi_sol_deactivate(intf, instance);
>> + }
>>
>> /*
>> * SOL loop test: Activate and then Dectivate
>> @@ -2083,7 +2112,7 @@
>> while (cnt > 0)
>> {
>> printf("remain loop test counter: %d\n", cnt);
>> - retval = ipmi_sol_activate(intf, 1, interval);
>> + retval = ipmi_sol_activate(intf, 1, interval, 1);
>> if (retval)
>> {
>> printf("SOL looptest failed: %d\n", retval);
>>
>>
>> ------------------------------------------------------------------------------
>> Everyone hates slow websites. So do we.
>> Make your web apps faster with AppDynamics
>> Download AppDynamics Lite for free today:
>> http://p.sf.net/sfu/appdyn_d2d_mar
>> _______________________________________________
>> Ipmitool-devel mailing list
>> Ipmitool-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
