On Tue, May 21, 2013 at 10:44 AM, Dmitry Bazhenov
<dim...@pigeonpoint.com> wrote:
> Hello, Zdenek,
>
> Can I assume that the patch has passed the review and will be applied to the
> mainline anytime soon?
Hello Dmitry,
well, I've looked at it. If you want to call it a review, sure :)
I'm wondering whether you insist on not checking the second strtol()
call. Also, whether you want to change 'rv' to unsigned long as
mentioned couple e-mails ago.
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?
> Or there are some actions from my side pending to be
> done?
None I know of.
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
