Re: [Qemu-devel] [PATCH] linux-user: improve target_to_host_sock_type conversion for MIPS

[Petar Jovanovic]

________________________________________
From: Aurelien Jarno address@hidden
Sent: Sunday, March 24, 2013 11:27 AM
To: Petar Jovanovic
Cc: address@hidden; Petar Jovanovic; address@hidden; Richard Henderson; Blue 
Swirl
Subject: Re: [PATCH] linux-user: improve target_to_host_sock_type conversion 
for MIPS

> Not related to your changes, but note that ARCH_HAS_SOCKET_TYPES is not
> defined anywhere contrary to what is said in the comment. It might be a
> good idea to revive it, as at least alpha and sparc also have the issue.

I have just revived it. It is defined for MIPS, ALPHA and SPARC.
I will update the patch with new changes today.

> +     * @SOCK_DGRAM - datagram (conn.less) socket
> +     * @SOCK_STREAM - stream (connection) socket
> +     * @SOCK_RAW - raw socket
> +     * @SOCK_RDM - reliably-delivered message
> +     * @SOCK_SEQPACKET - sequential packet socket
> +     * @SOCK_DCCP - Datagram Congestion Control Protocol socket
> +     * @SOCK_PACKET - linux specific way of getting packets at the dev level.
> +     *                For writing rarp and other similar things on the user
> +     *                level.
> +     * @SOCK_CLOEXEC - sets the close-on-exec (FD_CLOEXEC) flag.
> +     * @SOCK_NONBLOCK - sets the O_NONBLOCK file status flag.
> +     */
> +    enum sock_type {
> +           TARGET_SOCK_DGRAM       = 1,
> +           TARGET_SOCK_STREAM      = 2,
> +           TARGET_SOCK_RAW         = 3,
> +           TARGET_SOCK_RDM         = 4,
> +           TARGET_SOCK_SEQPACKET   = 5,
> +           TARGET_SOCK_DCCP        = 6,
> +           TARGET_SOCK_PACKET      = 10,
> +           TARGET_SOCK_CLOEXEC     = 02000000,
> +           TARGET_SOCK_NONBLOCK    = 0200,
> +    };
> +
> +    #define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)
> +    #define TARGET_SOCK_TYPE_MASK    0xf  /* Covers up to TARGET_SOCK_MAX-1. 
> */
>
>  #elif defined(TARGET_ALPHA)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ee82a2d..0a9e6db 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1859,30 +1859,38 @@ static void unlock_iovec(struct iovec *vec, abi_ulong 
> target_addr,
>      free(vec);
>  }
>
> -/* do_socket() Must return target values and target errnos. */
> -static abi_long do_socket(int domain, int type, int protocol)
> -{
>  #if defined(TARGET_MIPS)
> -    switch(type) {
> +static inline void target_to_host_sock_type(int *type)
> +{
> +    int host_type = 0;
> +    int target_type = *type;
> +
> +    switch (target_type & TARGET_SOCK_TYPE_MASK) {
>      case TARGET_SOCK_DGRAM:
> -        type = SOCK_DGRAM;
> +        host_type = SOCK_DGRAM;
>          break;
>      case TARGET_SOCK_STREAM:
> -        type = SOCK_STREAM;
> +        host_type = SOCK_STREAM;
>          break;
> -    case TARGET_SOCK_RAW:
> -        type = SOCK_RAW;
> -        break;
> -    case TARGET_SOCK_RDM:
> -        type = SOCK_RDM;
> -        break;
> -    case TARGET_SOCK_SEQPACKET:
> -        type = SOCK_SEQPACKET;
> -        break;
> -    case TARGET_SOCK_PACKET:
> -        type = SOCK_PACKET;
> +    default:
> +        host_type = target_type & TARGET_SOCK_TYPE_MASK;
>          break;
>      }

> I am not sure we want to really copy the type without more checking than
> the mask: a new value still within the mask limit could be added
> differently depending on the architecture.

I believe the current solution is applicable to all architectures that use
this function. 

> +    if (target_type & TARGET_SOCK_CLOEXEC) {
> +        host_type |= SOCK_CLOEXEC;
> +    }
> +    if (target_type & TARGET_SOCK_NONBLOCK) {
> +        host_type |= SOCK_NONBLOCK;
> +    }
> +    *type = host_type;

> Also I think the values should be checked in all cases returning -EINVAL
> if not supported. On other architecture where no translation is done,
> this is done by the host kernel. With the code above, unsupported
> arguments are simply ignored.

The kernel will return unsupported value if it does not support it.
One of the reasons to copy the type without more checking in other cases is
to let kernel return an error for unsupported types. Socket type is not a
'bit-flag', so conversion from target to host is tricky for irregular cases.

Petar