Hello, Zdenek,
Can I assume that the patch has passed the review and will be applied to
the mainline anytime soon? Or there are some actions from my side
pending to be done?
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
