Hi,
On Sat, Jul 17, 2010 at 11:37:34AM +0200, Emilio Pozuelo Monfort wrote:
> On 15/07/10 20:46, Carl Fredrik Hammar wrote:
> > On Thu, Jul 15, 2010 at 05:15:25PM +0200, Emilio Pozuelo Monfort wrote:
> > The first thing you should do in any RPC that is not a stub is:
> > if (!user)
> > return EOPNOTSUPP;
>
> Done. I'm not sure when user would be NULL. Do you know it?
Not really sure. Perhaps when a client calls a different kind of port
that doesn't have a corresponding sock_user, or perhaps there are some
border cases when creating or destroying the object. All I know is that
every single RPC I've seen does this.
> > Also I think you need to lock the user->sock->lock.
>
> As discussed on IRC, I wasn't sure about this since we're only reading
> variables, but your reasoning that the socket could be disconnected or
> whatever
> in the meantime makes sense and since I can't find any counter example on a
> quick look, I've done it.
>
> I'm locking it once before the switch to avoid locking it in many different
> cases. If that's not acceptable because some cases won't need it and we should
> avoid locking it in them (for performance reasons) I'll change it.
Yes, generally you should view not locking as an optimization:
only do it if it is worth the effort and you're sure it won't affect
the outcome.
> error_t
> S_socket_getopt (struct sock_user *user,
> int level, int opt,
> char **value, size_t *value_len)
> {
> - return EOPNOTSUPP;
> + int ret = 0;
> +
> + if (!user)
> + return EOPNOTSUPP;
> +
> + mutex_lock (&user->sock->lock);
> + switch (level)
> + {
> + case SOL_SOCKET:
> + switch (opt)
> + {
> + case SO_TYPE:
> + assert(*value_len >= sizeof (int));
Missing a space after assert.
> + *(int*)*value = user->sock->pipe_class->sock_type;
> + *value_len = sizeof (int);
> + break;
> + default:
> + ret = ENOPROTOOPT;
> + break;
> + }
> + break;
> + default:
> + ret = ENOPROTOOPT;
Hmm, at this level it probably shouldn't be ENOPROTOOPT since it's not
a protocol option, and since we it could still be a valid protocol level
we can't return EINVAL. Sorry, this was only clear to me once you split
the cases, guess it's back to EOPNOTSUPP here. :-)
Regards,
Fredrik
