Hello, Zdenek, Thanks for review. Please, see below.
19.05.2013 21:21, Zdenek Styblik пишет: > On Fri, May 17, 2013 at 1:52 PM, Dmitry Bazhenov <dim...@pigeonpoint.com> > wrote: >> Hello, All! >> > > Hello Dmitry, > > my comments are below. > >> I have updated the patch of direct serial drivers. Now single and double >> bridging is working. >> > > 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. > > 1] don't use atol() > 2] since you have included 'ipmitool/helper.h' use appropriate str2*() > function(s) instead of atol() and strtol() and be done with it. > > It seems to me like intf->session is leaking memory, resp. I don't see > it being freed anywhere. Fixed. > > There is a memory leak if '-D' is given more than once. Fixed. > > 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. > >> Also, I found several cases where "tmp_pass" string acquired from the >> "getpass()" call is incorrectly freed. The getpass() function returns >> address to a static buffer and freeing that address results in abrupt >> program termination. I removed these free() calls altogether. >> > > Great, and I mean great. But let's do one thing at the time, ok? What > I mean is it should, has to, be submitted separately. > Also, I think your patch is incorrect. If this is issue *only* for > getpass(), then fix should be something like: > ~~~ > #ifdef HAVE_GETPASSPHRASE > free(tmp_pass); > #endif > ~~~ These two functions both return a pointer to the static data. Hence, the #ifdef guard is unnecessary. 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 >> ------------------------------------------------------------------------------ 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
