Agreed.
28.05.2013 10:41, Zdenek Styblik пишет:
> 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
