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

Reply via email to