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
