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
>>>>>>>
>>>>>
>>>
>
------------------------------------------------------------------------------
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
