rtems_bsdnet_fdToSocket()

[Till Straumann]

Chris Johns wrote:
> 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. 
OK - then we're on the same page. I thought you didn't like the idea of
knowledge about a file type built into a 'iop'. If the type is encoded
in 'flags' or in a dedicated field doesn't matter too much, IMHO since
it would be accessed via a macro or inline, i.e., the user setting or
querying the type wouldn't have to know how/where is is kept.

rtems_libio_iop_set_type(iop, type)

type   rtems_libio_iop_get_type(iop)

> 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.
I couldn't agree more.
>
> 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.
Agreed, again.

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