Hello, Zdenek,
There's a typo (and it's mine originally):
enable_intf_serial=yes
Needed:
xenable_intf_serial=yes
Regards,
Dmitry
28.05.2013 10:35, Zdenek Styblik пишет:
> On Fri, May 24, 2013 at 9:24 AM, Dmitry Bazhenov <dim...@pigeonpoint.com>
> wrote:
>> Zdenek,
>>
>> Here it is.
>>
>> Regards,
>> Dmitry
>>
>
> @@ -195,6 +196,23 @@
>
> ORIG_CPPFLAGS=$CPPFLAGS
>
> +dnl enable serial interface
> +AC_ARG_ENABLE([intf-serial],
> + [AC_HELP_STRING([--enable-intf-serial],
> + [enable direct Serial Basic/Terminal mode
> interface [default=yes]])],
> + [xenable_intf_serial=$enableval], [xenable_intf_serial=yes])
> +if test "x$enable_intf_serial" = "xstatic" || test
> "x$enable_intf_serial" = "xplugin"; then
> + enable_intf_serial=yes
> +fi
> +if test "x$xenable_intf_serial" = "xyes"; then
> + AC_DEFINE(IPMI_INTF_SERIAL, [1], [Define to 1 to enable serial
> interface.])
> + AC_SUBST(INTF_SERIAL, [serial])
> + AC_SUBST(INTF_SERIAL_LIB, [libintf_serial.la])
> + IPMITOOL_INTF_LIB="$IPMITOOL_INTF_LIB serial/libintf_serial.la"
> +else
> + xenable_intf_serial=no
> +fi
> +
> dnl look for OpenIPMI header files
> AC_ARG_WITH([kerneldir],
> [AC_HELP_STRING([--with-kerneldir=DIR],
> @@ -583,7 +601,8 @@
>
> Agreed?
>
> Z.
>
>
>> 24.05.2013 13:14, Dmitry Bazhenov пишет:
>>>
>>> Hello, Zdenek,
>>>
>>>
>>> I'll provide the updated patch in a minute with your comments addressed.
>>> I'll add the double check you're talking about and remove the assert
>>> since they are redundant.
>>>
>>> Regards,
>>> Dmitry
>>>
>>> 22.05.2013 14:55, Zdenek Styblik пишет:
>>>>
>>>> On Tue, May 21, 2013 at 4:18 PM, Dmitry Bazhenov
>>>> <dim...@pigeonpoint.com> wrote:
>>>> [...]
>>>>>
>>>>> I would like not to place excessive check there in order not to
>>>>> overload the
>>>>> code. The surrounding context pretty much ensures that the conversion
>>>>> goes
>>>>> smoothly without errors.
>>>>>
>>>>
>>>> Dmitry,
>>>>
>>>> I still disagree, because there is no reason why not to double check
>>>> it. I guess I'm paranoid like that. However, this was a suggestion not
>>>> condition. If you say it's sufficient, then it's sufficient.
>>>>
>>>>>
>>>>>> Is there some extra documentation necessary/required (and available)
>>>>>> so this can be used/implemented/supported by other parties? But I
>>>>>> pretty much guessed this is just shoveling IPMI packets over serial
>>>>>> line, am I correct?
>>>>>
>>>>>
>>>>> You are correct. It's just a way to interact with IPM Controllers. I
>>>>> don't
>>>>> think that anything more than the help string is required here.
>>>>>
>>>>
>>>> Alright.
>>>>
>>>>>
>>>>> Good. Can you acknowledge that the patch will be submitted into the
>>>>> mainline?
>>>>>
>>>>
>>>> I don't know what exactly you meant by this question. If I'm the one
>>>> doing the commit, then yes. I have no problem to commit this into
>>>> ipmitool.
>>>>
>>>> Ok, I have one more question. serial_read_line() and
>>>> serial_write_line() have assert(str). Is this on purpose? Why? Are you
>>>> aware it's going to crash the program? I'm not in favour of doing such
>>>> thing. Since both functions return int, couldn't it be possible to do
>>>> without assert()?
>>>>
>>>> Thanks,
>>>> Z.
>>>>
>>>>>
>>>>> No more comments below.
>>>>>
>>>>> Regards,
>>>>> Dmitry
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Z.
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Dmitry
>>>>>>>
>>>>>>> 20.05.2013 19:32, Zdenek Styblik пишет:
>>>>>>>
>>>>>>>> On Mon, May 20, 2013 at 3:07 PM, Dmitry Bazhenov
>>>>>>>> <dim...@pigeonpoint.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Zdenek,
>>>>>>>>>
>>>>>>>>> I agree with the first fragment changes (probably, it is worth
>>>>>>>>> making
>>>>>>>>> "rv"
>>>>>>>>> an unsigned long).
>>>>>>>>>
>>>>>>>>
>>>>>>>> Dmitry,
>>>>>>>>
>>>>>>>> it was just a quick modification to show what I meant.
>>>>>>>>
>>>>>>>>> As for the second fragment, I suggest we remove any check at all.
>>>>>>>>> The
>>>>>>>>> surrounding context ensures that the converted string is a 2-digit
>>>>>>>>> hexadecimal (see the isxdigit(ch) check upper to the fragment).
>>>>>>>>>
>>>>>>>>
>>>>>>>> Uh. I'd be rather safe than sorry. My $0.02 USD.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Z.
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Dmitry
>>>>>>>>>
>>>>>>>>> 20.05.2013 18:50, Zdenek Styblik пишет:
>>>>>>>>>
>>>>>>>>>> On Mon, May 20, 2013 at 2:17 PM, Dmitry Bazhenov
>>>>>>>>>> <dim...@pigeonpoint.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hello, Zdenek,
>>>>>>>>>>>
>>>>>>>>>>> I've addressed your comments.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> I see. Well, this sucks. Two solutions come to my mind. The
>>>>>>>>>>>> first is
>>>>>>>>>>>> to extend str2*() calls which, I admit, is going to be a bit of
>>>>>>>>>>>> work.
>>>>>>>>>>>> The second is to check 'errno', endptr and '#include
>>>>>>>>>>>> <limits.h>'. In
>>>>>>>>>>>> other words, do what str2*() calls do.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I added checks of endptr and errno after the strtol() calls. In
>>>>>>>>>>> the
>>>>>>>>>>> context
>>>>>>>>>>> of these two calls these checks look sufficient to me. Please,
>>>>>>>>>>> review.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Dmitry
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Dmitry,
>>>>>>>>>>
>>>>>>>>>> please, see changes I've made/I propose. Lines which don't
>>>>>>>>>> being/aren't prefixed with '+' got either changed or added.
>>>>>>>>>> Also, some
>>>>>>>>>> parts of the code are missing, but since you have written it, it
>>>>>>>>>> shouldn't be a problem.
>>>>>>>>>> If you have any questions regarding changes I've made, please, ask.
>>>>>>>>>>
>>>>>>>>>> +static int
>>>>>>>>>> +recv_response(struct ipmi_intf * intf, unsigned char *data, int
>>>>>>>>>> len)
>>>>>>>>>> +{
>>>>>>>>>> + char hex_rs[IPMI_SERIAL_MAX_RESPONSE * 3];
>>>>>>>>>> int i, j, resp_len = 0;
>>>>>>>>>> /* This shouldn't matter since rv isn't used further
>>>>>>>>>> in the
>>>>>>>>>> code, is it? */
>>>>>>>>>> long int rv = 0;
>>>>>>>>>> unsigned long int hex_tmp = 0;
>>>>>>>>>> [...]
>>>>>>>>>> + if (strncmp(p, "ERR ", 4) == 0) {
>>>>>>>>>> + serial_write_line(intf, "\r\r\r\r");
>>>>>>>>>> + sleep(1);
>>>>>>>>>> + serial_flush(intf);
>>>>>>>>>> errno = 0;
>>>>>>>>>> rv = strtol(p + 4, &p, 16);
>>>>>>>>>> + if ((rv && rv < 0x100 && *p == '\0')
>>>>>>>>>> + || (rv == 0 && !errno)) {
>>>>>>>>>> + /* The message didn't get it through. The upper
>>>>>>>>>> + level will have to re-send */
>>>>>>>>>> + return 0;
>>>>>>>>>> + } else {
>>>>>>>>>> + lprintf(LOG_ERR, "Serial response is invalid");
>>>>>>>>>> + return -1;
>>>>>>>>>> + }
>>>>>>>>>> + }
>>>>>>>>>> + /* parse the response */
>>>>>>>>>> + i = 0;
>>>>>>>>>> + j = 0;
>>>>>>>>>> + while (*p) {
>>>>>>>>>> [...]
>>>>>>>>>> + if (j == 2) {
>>>>>>>>>> + /* parse the hex number */
>>>>>>>>>> + str_hex[j] = 0;
>>>>>>>>>> errno = 0;
>>>>>>>>>> hex_tmp = strtoul(str_hex, &pp, 16);
>>>>>>>>>> /* I'm not 100% sure about "|| *pp != '\0' "
>>>>>>>>>> here.
>>>>>>>>>> Please, test it! */
>>>>>>>>>> if (errno != 0 || hex_tmp > UCHAR_MAX || *pp
>>>>>>>>>> != '\0')
>>>>>>>>>> {
>>>>>>>>>> /* TODO - Handle this error */
>>>>>>>>>> }
>>>>>>>>>> data[i++] = hex_tmp;
>>>>>>>>>> + if (pp == str_hex || *pp != 0) {
>>>>>>>>>> + break;
>>>>>>>>>> + }
>>>>>>>>>> + j = 0;
>>>>>>>>>> + }
>>>>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> Z.
>>>>>>>>>>
>>>>>>>>>>> 20.05.2013 10:46, Zdenek Styblik пишет:
>>>>>>>>>>>
>>>>>>>>>>>> On Mon, May 20, 2013 at 6:31 AM, Dmitry Bazhenov
>>>>>>>>>>>> <dim...@pigeonpoint.com>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Great. However, please fix the following lines:
>>>>>>>>>>>>>> + rate = atol(p + 1);
>>>>>>>>>>>>>> + rate = atol(p + 1);
>>>>>>>>>>>>>> + rv = strtol(p + 4, &p, 16);
>>>>>>>>>>>>>> + data[i++] = (unsigned char)
>>>>>>>>>>>>>> strtol(str_hex,
>>>>>>>>>>>>>> &pp,
>>>>>>>>>>>>>> 16);
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have changed atol() calls to str2uint().
>>>>>>>>>>>>> Unfortunately, ipmitool/helper.h does not provide appropriate
>>>>>>>>>>>>> API
>>>>>>>>>>>>> to
>>>>>>>>>>>>> replace
>>>>>>>>>>>>> strtol(). In the code strtol() is called with radix=16 that
>>>>>>>>>>>>> means
>>>>>>>>>>>>> the
>>>>>>>>>>>>> input
>>>>>>>>>>>>> is read in hexadecimal form only. I think it is best to leave
>>>>>>>>>>>>> the
>>>>>>>>>>>>> strtol()
>>>>>>>>>>>>> calls there as they are.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I see. Well, this sucks. Two solutions come to my mind. The
>>>>>>>>>>>> first is
>>>>>>>>>>>> to extend str2*() calls which, I admit, is going to be a bit of
>>>>>>>>>>>> work.
>>>>>>>>>>>> The second is to check 'errno', endptr and '#include
>>>>>>>>>>>> <limits.h>'. In
>>>>>>>>>>>> other words, do what str2*() calls do.
>>>>>>>>>>>>
>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'm also wondering whether /dev/tty or /dev/ttyS couldn't be
>>>>>>>>>>>>>> implied
>>>>>>>>>>>>>> and whether extending 'struct ipmi_intf' is really
>>>>>>>>>>>>>> necessary. But
>>>>>>>>>>>>>> it's
>>>>>>>>>>>>>> just a silly question, I guess.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I couldn't find a better solution to provide a device string
>>>>>>>>>>>>> to the
>>>>>>>>>>>>> drivers.
>>>>>>>>>>>>> The implying /dev/tty(s, USB) strings in the command-line is
>>>>>>>>>>>>> possible.
>>>>>>>>>>>>> I'll
>>>>>>>>>>>>> think through how to make it without affecting those users
>>>>>>>>>>>>> who get
>>>>>>>>>>>>> used
>>>>>>>>>>>>> to
>>>>>>>>>>>>> specify the full device names.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> No, you're right. Serial dev can have <whatever> name. I guess
>>>>>>>>>>>> it's
>>>>>>>>>>>> fine as it's.
>>>>>>>>>>>>
>>>>>>>>>>>> [...]
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> These two functions both return a pointer to the static data.
>>>>>>>>>>>>> Hence,
>>>>>>>>>>>>> the
>>>>>>>>>>>>> #ifdef guard is unnecessary.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Hmm :-S Ok. To be honest, I'm disappointed, because this means
>>>>>>>>>>>> possible memory leak which "can't" be fixed.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Z.
>>>>>>>>>>>>
>>>>>>>>>>>>> I'll provide the updated patch for serial drivers and separate
>>>>>>>>>>>>> patch
>>>>>>>>>>>>> for
>>>>>>>>>>>>> the
>>>>>>>>>>>>> bug soon.
>>>>>>>>>>>>>
>>>>>>>>>>>>> No more comments below.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>> Dmitry
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Z.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Please, review.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>> Dmitry
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 26.04.2013 16:08, Dmitry Bazhenov пишет:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hello, all,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I would like to submit a patch which adds Serial Basic and
>>>>>>>>>>>>>>>> Terminal
>>>>>>>>>>>>>>>> mode
>>>>>>>>>>>>>>>> interface drivers which utilize a direct serial connection in
>>>>>>>>>>>>>>>> order
>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>> interact with IPMC.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Both the patches implement single and double bridging.
>>>>>>>>>>>>>>>> However,
>>>>>>>>>>>>>>>> bridging
>>>>>>>>>>>>>>>> is temporarily broken for PICMG systems due to the known
>>>>>>>>>>>>>>>> bridging
>>>>>>>>>>>>>>>> issues. I'm going to update the patch as soon as these issues
>>>>>>>>>>>>>>>> are
>>>>>>>>>>>>>>>> resolved.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>> Dmitry
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> AlienVault Unified Security Management (USM) platform delivers
>>>>>>>>>>>>>>> complete
>>>>>>>>>>>>>>> security visibility with the essential security capabilities.
>>>>>>>>>>>>>>> Easily
>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>> efficiently configure, manage, and operate all of your
>>>>>>>>>>>>>>> security
>>>>>>>>>>>>>>> controls
>>>>>>>>>>>>>>> from a single console and one unified framework. Download a
>>>>>>>>>>>>>>> free
>>>>>>>>>>>>>>> trial.
>>>>>>>>>>>>>>> http://p.sf.net/sfu/alienvault_d2d
>>>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>>>> Ipmitool-devel mailing list
>>>>>>>>>>>>>>> Ipmitool-devel@lists.sourceforge.net
>>>>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>
------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
