On Mon, Mar 25, 2013 at 5:01 PM, Corey Minyard <tcminy...@gmail.com> wrote:
> On 03/25/2013 05:29 AM, Zdenek Styblik wrote:
>>
>> 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'?
>
>
> Well, yes, but that can often lead to subtle bugs and reduced performance.
> It's generally best to use the natural integer unless you really need a
> reduced size.
>
Alright. I guess my drive for saving resources(or lack of experience)
tends to over-do things from time to time.
[...]
>>> ~~~
>>> - 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)?
>
>
> Laziness on my part :). I can fix that. The UI for that particular option
> is, well, not ideal, and that's why I didn't fix this at first.
>
I wasn't sure whether this was on purpose or bug. It's better to ask
sooner than later ;)
Anyway, I'm ok with proposed changes. Please, send me the diff file
and I'll commit it. I don't like to scrape diffs off the e-mail.
Thanks,
Z.
> -corey
>
>
>>> Best regards,
>>> Z.
>>>
>> Ignore previous e-mail as it had 'instance > 14' in it which is wrong.
>> I don't know why I put '14' there.
>>
>> One more thing. I think this check ``if (instance < 1 || instance >
>> 15) { /* Error */ }'' should be moved into ipmi_sol_activate(), resp.
>> ipmi_sol_deactivate() or perhaps even create new function(wrapper) for
>> it.
>
>
> Yes, that's a good idea. New patch is appended:
>
> -corey
>
>
> 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 25 Mar 2013 15:56:51 -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 25 Mar 2013 15:56:51 -0000
> @@ -1292,12 +1292,18 @@
>
> * 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;
> uint8_t data[6];
>
> + if ((instance <= 0) || (instance > 15))
> + {
> + lprintf(LOG_ERR, "Error: Instance must range from 1 to 15");
>
> + return -1;
> + }
> +
> memset(&req, 0, sizeof(req));
> req.msg.netfn = IPMI_NETFN_APP;
> req.msg.cmd = IPMI_DEACTIVATE_PAYLOAD;
> @@ -1305,8 +1311,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 +1539,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 +1685,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 +1695,7 @@
>
> exit(1);
> }
> else
> - ipmi_sol_deactivate(intf);
> + ipmi_sol_deactivate(intf, instance);
>
> return 0;
> }
> @@ -1701,7 +1707,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;
> @@ -1721,6 +1728,12 @@
> return -1;
> }
>
> + if ((instance <= 0) || (instance > 15))
> + {
> + lprintf(LOG_ERR, "Error: Instance must range from 1 to 15");
>
> + return -1;
> + }
> +
>
> /*
> * Setup a callback so that the lanplus processing knows what
> @@ -1737,7 +1750,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 +1856,7 @@
>
>
> if(looptest == 1)
> {
> - ipmi_sol_deactivate(intf);
> + ipmi_sol_deactivate(intf, instance);
> usleep(interval*1000);
> return 0;
> }
> @@ -1854,7 +1867,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,9 +1887,9 @@
>
> 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, " looptest [<loop times>] [<loop
> interval(in ms)>]");
>
> + lprintf(LOG_NOTICE, " activate
> [<usesolkeepalive|nokeepalive>] [instance=<number>]");
> + lprintf(LOG_NOTICE, " deactivate [instance=<number>]");
> + lprintf(LOG_NOTICE, " looptest [<loop times> [<loop
> interval(in ms)> [<instance>]]]");
> }
>
>
> @@ -2029,32 +2042,54 @@
>
> * Activate
> */
> else if (!strncmp(argv[0], "activate", 8)) {
> + int i;
> + uint8_t 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)) {
> + if (str2uchar(argv[i] + 9, &instance) != 0) {
>
> + 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;
> + uint8_t instance = 1;
>
> +
> + for (i = 1; i < argc; i++) {
> + if (!strncmp(argv[i], "instance=", 9)) {
> + if (str2uchar(argv[i] + 9, &instance) != 0) {
>
> + 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
> @@ -2063,8 +2098,9 @@
> {
> int cnt = 200;
> int interval = 100; /* Unit is: ms */
> + uint8_t instance;
>
> - if (argc > 3)
> + if (argc > 4)
> {
> print_sol_usage();
> return -1;
> @@ -2074,16 +2110,25 @@
> cnt = strtol(argv[1], NULL, 10);
> if(cnt <= 0) cnt = 200;
> }
> - if (argc == 3)
> + if (argc >= 3)
> {
> interval = strtol(argv[2], NULL, 10);
> if(interval < 0) interval = 0;
> }
> + if (argc >= 4)
> + {
> + if (str2uchar(argv[3], &instance) != 0) {
> + lprintf(LOG_ERR, "instance invalid: '%s'",
> + argv[3]);
>
> + print_sol_usage();
> + return -1;
> + }
> + }
>
> 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, instance);
>
> if (retval)
> {
> printf("SOL looptest failed: %d\n", retval);
>
------------------------------------------------------------------------------
Own the Future-Intel® Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest.
Compete for recognition, cash, and the chance to get your game
on Steam. $5K grand prize plus 10 genre and skill prizes.
Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
