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
