Hello, Zdenek,
I'll provide the updated patch in a minute with your comments addressed.
I'll add the double check you're talking about and remove the assert
since they are redundant.
Regards,
Dmitry
22.05.2013 14:55, Zdenek Styblik пишет:
> On Tue, May 21, 2013 at 4:18 PM, Dmitry Bazhenov <dim...@pigeonpoint.com>
> wrote:
> [...]
>> 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.
>>
>
> Dmitry,
>
> I still disagree, because there is no reason why not to double check
> it. I guess I'm paranoid like that. However, this was a suggestion not
> condition. If you say it's sufficient, then it's sufficient.
>
>>
>>> 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.
>>
>
> Alright.
>
>>
>> Good. Can you acknowledge that the patch will be submitted into the
>> mainline?
>>
>
> I don't know what exactly you meant by this question. If I'm the one
> doing the commit, then yes. I have no problem to commit this into
> ipmitool.
>
> Ok, I have one more question. serial_read_line() and
> serial_write_line() have assert(str). Is this on purpose? Why? Are you
> aware it's going to crash the program? I'm not in favour of doing such
> thing. Since both functions return int, couldn't it be possible to do
> without assert()?
>
> Thanks,
> Z.
>
>>
>> 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
