[zones-discuss] s10 brand code review
I need a code review for the fix for: ssh dumping core on maramba There is a webrev at: http://cr.opensolaris.org/~gjelinek/webrev.crypto/ Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand code review
- you might want to add the crypto libraries to the list of libraries to watch for breakage. ;) - to improve the readability of crypto_ioctl(), could you make a dedicated function or macro that just does translation between the s10 and nevada versions of crypto_get_function_list_t? - when you make the ioctl call, i see you can do a CT_TSET. in this scenario you only initialize native_param.fl_provider_id, and the rest of native_param is garbage. is that ok or should the rest of native_param be zeroed out? ed On Mon, Jun 15, 2009 at 10:39:39AM -0600, Jerry Jelinek wrote: > I need a code review for the fix for: > > ssh dumping core on maramba > > There is a webrev at: > > http://cr.opensolaris.org/~gjelinek/webrev.crypto/ > > Thanks, > Jerry > ___ > zones-discuss mailing list > zones-discuss@opensolaris.org ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand code review
Ed, Thanks for reviewing this. Edward Pilatowicz wrote: - you might want to add the crypto libraries to the list of libraries to watch for breakage. ;) Yep. :-) I'm also going to try to see if any of the other default devices that are inside of a zone might have had their ioctl params changes in any way. - to improve the readability of crypto_ioctl(), could you make a dedicated function or macro that just does translation between the s10 and nevada versions of crypto_get_function_list_t? Will do. - when you make the ioctl call, i see you can do a CT_TSET. in this scenario you only initialize native_param.fl_provider_id, and the rest of native_param is garbage. is that ok or should the rest of native_param be zeroed out? For CT_TSET that goes through the new ctfs_ioctl() function, not this new code for /dev/crypto. That function is the original code Jordan did which I just moved out into its own function for readability. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand code review
On Mon, Jun 15, 2009 at 11:54:46AM -0600, Jerry Jelinek wrote: > Ed, > > Thanks for reviewing this. > > Edward Pilatowicz wrote: >> - you might want to add the crypto libraries to the list of libraries >> to watch for breakage. ;) > > Yep. :-) I'm also going to try to see if any of the other default devices > that are inside of a zone might have had their ioctl params changes > in any way. > >> - to improve the readability of crypto_ioctl(), could you make a >> dedicated function or macro that just does translation between >> the s10 and nevada versions of crypto_get_function_list_t? > > Will do. > >> - when you make the ioctl call, i see you can do a CT_TSET. in >> this scenario you only initialize native_param.fl_provider_id, >> and the rest of native_param is garbage. is that ok or should >> the rest of native_param be zeroed out? > > For CT_TSET that goes through the new ctfs_ioctl() function, > not this new code for /dev/crypto. That function is the original > code Jordan did which I just moved out into its own function > for readability. > oops. my bad. ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand code review
On 06/15/09 09:39, Jerry Jelinek wrote: I need a code review for the fix for: ssh dumping core on maramba There is a webrev at: http://cr.opensolaris.org/~gjelinek/webrev.crypto/ Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org Hi Jerry, I have a few nits. Some of them (such at [3]) might be ridiculously trivial, so I won't complain if you don't take them into account. -- 1. If you're moving my CTFS ioctl emulation code into its own function, then the code can be greatly simplified by lifting the code shared by TC_TGET and TC_TSET out of the large switch block. You can then change the switch block into a conditional statement: if (cmd == TC_TGET && s10_uucopy(&s10param, (void *)arg, sizeof (s10param)) != 0) return (EFAULT); return (0); 2. You can collapse the CT_TGET and CT_TSET cases in s10_ioctl() into a single case block for both ioctl commands because they both call ctfs_ioctl() with identical parameters. 3. I noticed weeks ago that there are many places in the brand emulation library where code of the form if ((err = F(...)) != 0) return (err); return (0); (where F(...) is any function invocation) can be transformed into the following equivalent code: return (F(...)); This happens a few times in the code that you modified. Applying this transformation might make the code slightly more readable. I filed a mental note so that I'll do the same in all of my future additions to the emulation library. -- BTW, I like your use of static local variables in crypto_ioctl(). I haven't seen static local variables since I created singleton C++ classes years ago. :) Jordan ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand code review
Jordan Vaughan wrote: I have a few nits. Some of them (such at [3]) might be ridiculously trivial, so I won't complain if you don't take them into account. Jordan, Thanks for reviewing this. I'll make all of the simplifications you suggested. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
