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: Emilio Pozuelo Monfort
Subject: Re: [PATCH] Implement getsockopt (fd, SOL_SOCKET, SO_TYPE, ...)
Date: Sat, 17 Jul 2010 11:37:34 +0200
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.10) Gecko/20100619 Icedove/3.0.5

Hi,

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?

> 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.

>> +  switch (level)
>> +    {
>> +    case SOL_SOCKET:
>> +      switch (opt)
>> +    {
>> +    case SO_TYPE:
>> +      if (value_len == NULL || value == NULL || *value == NULL
>> +        || value_len < sizeof (user->sock->pipe_class->sock_type))
>> +        return EINVAL;
> 
> Beyond the handling size as described in POSIX, you don't need to test
> any tests for NULL because pointers doesn't come from the client itself
> but from MIG which gives you pointers into the reply message buffer.

I thought about that but since I wasn't completely sure I stayed in the safe
side :) I've nuked it now.

> In fact, even *value_len isn't passed from the client it's the size of
> the buffer in the message which is hard coded to 2048 at the moment.
> The code in pfinet doesn't do any checks either and you can even see
> this directly by looking at hurd/RPC_socket_getopt.c in a glibc build
> and pflocal/socketUser.c in a Hurd build.
> 
> So it seems you don't need to worry about size at all but I'd put in an
> assert(*value_len >= sizeof(int)) just to be safe, and perhaps comments
> for the pointers never being NULL.  You still need to set *value_len
> though.

I've added the assert.

> Also you might as well just use sizeof (int) since this is specified
> by POSIX:
> http://www.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_10_16

Yeah, and that's more correct since I'm doing *(int*). Changed it.

>> +      *(int*)*value = user->sock->pipe_class->sock_type;
>> +      *value_len = sizeof (user->sock->pipe_class->sock_type);
>> +      return 0;
>> +    default:
>> +      break;
>> +    }
>> +    default:
>> +      break;
>> +    }
>> +
>>    return EOPNOTSUPP;
>>  }
> 
> I think you should return ENOPROTOOPT here.  Don't forget to unlock.  ;-)

Right, that error is the good one here.

Here's an updated patch. I've tested it briefly and works fine.

Cheers,
Emilio

>From 9b0d6a4cdaff18067064fb67dc37e7ac8300a2f3 Mon Sep 17 00:00:00 2001
From: Emilio Pozuelo Monfort <pochu27@gmail.com>
Date: Wed, 14 Jul 2010 18:40:36 +0200
Subject: [PATCH] Implement getsockopt (fd, SOL_SOCKET, SO_TYPE)

        * pflocal/socket.c (S_socket_getopt): Add SO_TYPE support.
---
 pflocal/socket.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/pflocal/socket.c b/pflocal/socket.c
index 06777ca..94ed7ee 100644
--- a/pflocal/socket.c
+++ b/pflocal/socket.c
@@ -1,6 +1,6 @@
 /* Socket-specific operations

-   Copyright (C) 1995, 2008 Free Software Foundation, Inc.
+   Copyright (C) 1995, 2008, 2010 Free Software Foundation, Inc.

    Written by Miles Bader <miles@gnu.ai.mit.edu>

@@ -410,15 +410,40 @@ S_socket_recv (struct sock_user *user,

   return err;
 }
-

-/* Stubs for currently unsupported rpcs.  */

 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));
+         *(int*)*value = user->sock->pipe_class->sock_type;
+         *value_len = sizeof (int);
+         break;
+       default:
+         ret = ENOPROTOOPT;
+         break;
+       }
+      break;
+    default:
+      ret = ENOPROTOOPT;
+      break;
+    }
+  mutex_unlock (&user->sock->lock);
+
+  return ret;
 }

 error_t
-- 
1.7.1



reply via email to

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