On Tue, May 28, 2013 at 6:39 AM, Dmitry Bazhenov <dim...@pigeonpoint.com> wrote:
> Hello, Zdenek,
>
> There's a typo (and it's mine originally):
>
> enable_intf_serial=yes
>
> Needed:
>
> xenable_intf_serial=yes
>
> Regards,
> Dmitry
>
Oh, I see Dmitry. However, the changes I've made seem to be required
anyway, otherwise it gets borked. Please, check again.
Thanks,
Z.
> 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
