Re: Socket options KPI

On Fri, Jan 18, 2008 at 07:08:37PM +0200, Elad Efrat wrote:
> All comments are welcome. It's not planned on being
committed anytime
> soon (unless, of course, all feedback suggests
otherwise), so take your
> time carefully reviewing the changes if you're
interested. 

so_setsockopt:
Shouldn't valsize be the normal size_t? Does socklen_t
really make sense
here? The val pointer should be const.

sockopt_ensure_writeable --> I don't like the name.

If you can now also push the socket options down into
ipv4/ipv6 and the
patch can go without the sockopt_setmbuf/getmbuf, that would
be very
nice 

Without a full review, this looks like a huge improvement.

Joerg

Joerg Sonnenberger wrote:

> so_setsockopt:
> Shouldn't valsize be the normal size_t? Does socklen_t
really make sense
> here? 

I vaguely remember there was a reason for this socklen_t...
maybe the 
original FreeBSD changes. Anyway, I'm thinking it should be
unsigned
int, actually, to fit the len parameter to sys_setsockopt()
and
sys_getsockopt(), no?

> The val pointer should be const.

Fixed.

> sockopt_ensure_writeable --> I don't like the name.

I don't mind the naming. Bring up any name you'd like, and
if people
agree, I'll change.

> If you can now also push the socket options down into
ipv4/ipv6 and the
> patch can go without the sockopt_setmbuf/getmbuf, that
would be very
> nice 

Yes. Unfortunately, some code is *very* hairy about that
kinda stuff so
I'm trying to avoid it in the first pass (see, for example,
netiso).

> Without a full review, this looks like a huge
improvement.

Great to hear! 

Thanks,

-e.