From: Zdenek S. <zde...@gm...> - 2013-05-20 13:33:03
|
On Mon, May 20, 2013 at 3:07 PM, Dmitry Bazhenov <di...@pi...> 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 <di...@pi...> >> 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 >>>> <di...@pi...> >>>> 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 >>>>>>> Ipm...@li... >>>>>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel >>>>>>> >>>>> >>> > |