bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Implement getsockopt (fd, SOL_SOCKET, SO_TYPE, ...)


From: Carl Fredrik Hammar
Subject: Re: [PATCH] Implement getsockopt (fd, SOL_SOCKET, SO_TYPE, ...)
Date: Sat, 17 Jul 2010 12:25:48 +0200
User-agent: Mutt/1.5.20 (2009-06-14)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]