rtems_bsdnet_fdToSocket()

[Chris Johns]

Joel Sherrill wrote:
> Till Straumann wrote:
>> Chris Johns wrote:
>>  
>>> Till Straumann wrote:
>>>    
>>>> IMHO rtems_bsdnet_fdToSocket() doesn't
>>>> verify thoroughly enough that the file
>>>> descriptor passed in is really a socket.
>>>>
>>>> If you e.g., try getsockname() on a fd
>>>> that was opened on a termios device then
>>>> a crash may result.
>>>>
>>>> I propose to let rtems_bsdnet_fdToSocket()
>>>> test not only that iop->data1 is NULL but
>>>> to add an additional test.
>>>>
>>>> Either
>>>> 1) add a new flag LIBIO_FLAGS_ISSOCK
>>>>    which is set by rtems_bsdnet_makeFdForSocket()
>>>>    and tested by rtems_bsdnet_fdToSocket()
>>>> 1a) instead of one flag we could use a few
>>>>    more bits and create a LIBIO_FLAGS_IOP_TYPE
>>>>    bitmask (in case the need for other special
>>>>    cases besides sockets arises in the future).
>>>>    The test would then look like
>>>>
>>>>    if ( LIBIO_FLAGS_IOP_TYPE(iop->flags) != 
>>>> LIBIO_FLAGS_IOP_TYPE_SOCK ) {
>>>>        errno = ENOTSOCK;
>>>>        return NULL;
>>>>    }
>>>>
>>>>    Sidenote: IMO, if data1 == NULL then errno should
>>>>              also be ENOTSOCK instead of EBADF.
>>>>
>>>>       
>>> The LIBIO flags do not describe the fd rather they deal with what you 
>>> can do. I do not like the type of fd being added to the flags.
>>>
>>> NetBSD has a type field in the fp referenced by the fd:
>>>     
>> Problemo being that libio has no such thing. It has
>> absolutely no knowledge about what is attached to
>> 'data', 'data1', 'file-info'.
>>
>> Where do you see an equivalent to fp_type in the iop
>> or how would you propose that, given a iop, the
>> code can figure out what kind of object
>> is attached to e.g., 'data1' ?
>>

I am suggesting we add a type field. This makes the way we test common and 
does not depend on the way the other fields in the struct are used. I see the 
other ways as possible maintenance issues. That is the tests for a specific 
type cannot overlap and all need to be in a common location. Making the test 
common is even better, ie check the type field. This is a limited number of types.

Again NetBSD has:
$ less sys/sys/file.h
   .....
/*
  * Kernel descriptor table.
  * One entry for each open kernel vnode and socket.
  */
struct file {
         LIST_ENTRY(file) f_list;        /* list of active files */
         int             f_flag;         /* see fcntl.h */
         int             f_iflags;       /* internal flags */
#define DTYPE_VNODE     1               /* file */
#define DTYPE_SOCKET    2               /* communications endpoint */
#define DTYPE_PIPE      3               /* pipe */
#define DTYPE_KQUEUE    4               /* event queue */
#define DTYPE_MISC      5               /* misc file descriptor type */
#define DTYPE_CRYPTO    6               /* crypto */
#define DTYPE_NAMES \
     "0", "file", "socket", "pipe", "kqueue", "misc", "crypto"
         int             f_type;         /* descriptor type */
   ......

I also like the iflags field. If we make a change to add fields I would like 
the iflags field added.

>> Do you like testing the 'handlers' better?

No. It exposes the lower layers to the upper layers and means more globals.

There could also be other complexity like something having more than one 
possible set of handlers. The DOSFS has this for dir and file handlers.

>>   
> Looking at libcsupport/src/libio_socket.c, that's the only
> thing you can currently rely on.
> 
> Might not be a bad generic check.  Worth a libio macro
> like rtems_libio_handlers_match?

It mixes layers. What handlers do you need to match with.

Regards
Chris

> 
> --joel
>> T.
>>  
>>> int
>>> getsock(struct filedesc *fdp, int fdes, struct file **fpp)
>>> {
>>>         struct file     *fp;
>>>
>>>         if ((fp = fd_getfile(fdp, fdes)) == NULL)
>>>                 return (EBADF);
>>>
>>>         FILE_USE(fp);
>>>
>>>         if (fp->f_type != DTYPE_SOCKET) {
>>>                 FILE_UNUSE(fp, NULL);
>>>                 return (ENOTSOCK);
>>>         }
>>>         *fpp = fp;
>>>         return (0);
>>> }
>>>
>>> I would prefer this.
>>>
>>> Regards
>>> Chris
>>>
>>>    
>>>> 2) alternatively, rtems_bsdnet_fdToSocket() could
>>>>    test if iop->handlers == socket_handlers.
>>>>
>>>>    socket_handlers would then have to become a globally
>>>>    visible symbol (right now a static variable).
>>>>
>>>>    Unfortunately, this would introduce some asymmetry
>>>>    (fdToSocket() refers to global symbol, makeFdForSocket
>>>>    gets pointer to handlers passed in).
>>>>
>>>> If nobody objects or has a better idea then I would
>>>> implement 1a), create a PR and commit to 4.9 and HEAD.
>>>>
>>>> RFC
>>>> -Till
>>>> _______________________________________________
>>>> rtems-users mailing list
>>>> rtems-users at rtems.com
>>>> http://rtems.rtems.org/mailman/listinfo/rtems-users
>>>>       
>>
>> _______________________________________________
>> rtems-users mailing list
>> rtems-users at rtems.com
>> http://rtems.rtems.org/mailman/listinfo/rtems-users
>>   
> 
>