Zdenek,
Please, see below:
21.05.2013 16:45, Zdenek Styblik пишет:
> 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.
I would like not to place excessive check there in order not to overload
the code. The surrounding context pretty much ensures that the
conversion goes smoothly without errors.
> 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?
You are correct. It's just a way to interact with IPM Controllers. I
don't think that anything more than the help string is required here.
>
>> Or there are some actions from my side pending to be
>> done?
>
> None I know of.
Good. Can you acknowledge that the patch will be submitted into the
mainline?
No more comments below.
Regards,
Dmitry
>
> 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
