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
