Zdenek,
I agree with the first fragment changes (probably, it is worth making
"rv" an unsigned long).
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).
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
