Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user.

[Riku Voipio <riku.voipio@iki.fi>]

On Tue, Apr 21, 2009 at 10:21:38AM +0200, Arnaud Patard wrote:
> Ok, done. New patch attached. Please comment :)

thanks, this is already better. just two comments.

> Index: qemu/linux-user/syscall.c
> ===================================================================
> --- qemu.orig/linux-user/syscall.c
> +++ qemu/linux-user/syscall.c
> @@ -253,6 +253,18 @@ static bitmask_transtbl fcntl_flags_tbl[
>    { 0, 0, 0, 0 }
>  };
>  
> +static char target_to_host_fcntl_cmd[] = {
> +  [ TARGET_F_GETLK    ] = F_GETLK,
> +  [ TARGET_F_SETLK    ] = F_SETLK,
> +  [ TARGET_F_SETLKW   ] = F_SETLKW,
> +  [ TARGET_F_GETLK64  ] = F_GETLK64,
> +  [ TARGET_F_SETLK64  ] = F_SETLK64,
> +  [ TARGET_F_SETLKW64 ] = F_SETLKW64,
> +  [ TARGET_F_SETOWN   ] = F_SETOWN,
> +  [ TARGET_F_GETOWN   ] = F_GETOWN,
> +  [ TARGET_F_SETSIG   ] = F_SETSIG,
> +  [ TARGET_F_GETSIG   ] = F_GETSIG,
> +};

explict size for table and check for not overflowing?

>  #define COPY_UTSNAME_FIELD(dest, src) \
>    do { \
>        /* __NEW_UTS_LEN doesn't include terminating null */ \
> @@ -3365,6 +3377,7 @@ static abi_long do_fcntl(int fd, int cmd
>      struct flock64 fl64;
>      struct target_flock64 *target_fl64;
>      abi_long ret;
> +    int cmd2 = target_to_host_fcntl_cmd[cmd];

cmd2 is not a very good variable name, for example host_cmd
would describe it better.

>      switch(cmd) {
>      case TARGET_F_GETLK:
> @@ -3376,7 +3389,7 @@ static abi_long do_fcntl(int fd, int cmd
>          fl.l_len = tswapl(target_fl->l_len);
>          fl.l_pid = tswapl(target_fl->l_pid);
>          unlock_user_struct(target_fl, arg, 0);
> -        ret = get_errno(fcntl(fd, cmd, &fl));
> +        ret = get_errno(fcntl(fd, cmd2, &fl));
>          if (ret == 0) {
>              if (!lock_user_struct(VERIFY_WRITE, target_fl, arg, 0))
>                  return -TARGET_EFAULT;
> @@ -3399,7 +3412,7 @@ static abi_long do_fcntl(int fd, int cmd
>          fl.l_len = tswapl(target_fl->l_len);
>          fl.l_pid = tswapl(target_fl->l_pid);
>          unlock_user_struct(target_fl, arg, 0);
> -        ret = get_errno(fcntl(fd, cmd, &fl));
> +        ret = get_errno(fcntl(fd, cmd2, &fl));
>          break;
>  
>      case TARGET_F_GETLK64:
> @@ -3411,7 +3424,7 @@ static abi_long do_fcntl(int fd, int cmd
>          fl64.l_len = tswapl(target_fl64->l_len);
>          fl64.l_pid = tswap16(target_fl64->l_pid);
>          unlock_user_struct(target_fl64, arg, 0);
> -        ret = get_errno(fcntl(fd, cmd >> 1, &fl64));
> +        ret = get_errno(fcntl(fd, cmd2, &fl64));
>          if (ret == 0) {
>              if (!lock_user_struct(VERIFY_WRITE, target_fl64, arg, 0))
>                  return -TARGET_EFAULT;
> @@ -3433,18 +3446,25 @@ static abi_long do_fcntl(int fd, int cmd
>          fl64.l_len = tswapl(target_fl64->l_len);
>          fl64.l_pid = tswap16(target_fl64->l_pid);
>          unlock_user_struct(target_fl64, arg, 0);
> -        ret = get_errno(fcntl(fd, cmd >> 1, &fl64));
> +        ret = get_errno(fcntl(fd, cmd2, &fl64));
>          break;
>  
> -    case F_GETFL:
> -        ret = get_errno(fcntl(fd, cmd, arg));
> +    case TARGET_F_GETFL:
> +        ret = get_errno(fcntl(fd, cmd2, arg));
>          if (ret >= 0) {
>              ret = host_to_target_bitmask(ret, fcntl_flags_tbl);
>          }
>          break;
>  
> -    case F_SETFL:
> -        ret = get_errno(fcntl(fd, cmd, target_to_host_bitmask(arg, fcntl_flags_tbl)));
> +    case TARGET_F_SETFL:
> +        ret = get_errno(fcntl(fd, cmd2, target_to_host_bitmask(arg, fcntl_flags_tbl)));
> +        break;
> +
> +    case TARGET_F_SETOWN:
> +    case TARGET_F_GETOWN:
> +    case TARGET_F_SETSIG:
> +    case TARGET_F_GETSIG:
> +        ret = get_errno(fcntl(fd, cmd2, arg));
>          break;
>  
>      default:
> @@ -6222,20 +6242,7 @@ abi_long do_syscall(void *cpu_env, int n
>  	struct target_eabi_flock64 *target_efl;
>  #endif
>  
> -        switch(arg2){
> -        case TARGET_F_GETLK64:
> -            cmd = F_GETLK64;
> -            break;
> -        case TARGET_F_SETLK64:
> -            cmd = F_SETLK64;
> -            break;
> -        case TARGET_F_SETLKW64:
> -            cmd = F_SETLK64;
> -            break;
> -        default:
> -            cmd = arg2;
> -            break;
> -        }
> +	cmd = target_to_host_fcntl_cmd[arg2];
>  
>          switch(arg2) {
>          case TARGET_F_GETLK64:
> @@ -6315,7 +6322,7 @@ abi_long do_syscall(void *cpu_env, int n
>              ret = get_errno(fcntl(arg1, cmd, &fl));
>  	    break;
>          default:
> -            ret = do_fcntl(arg1, cmd, arg3);
> +            ret = do_fcntl(arg1, arg2, arg3);
>              break;
>          }
>  	break;
> Index: qemu/linux-user/syscall_defs.h
> ===================================================================
> --- qemu.orig/linux-user/syscall_defs.h
> +++ qemu/linux-user/syscall_defs.h
> @@ -1662,6 +1662,12 @@ struct target_statfs64 {
>  #define TARGET_F_SETLKW        9
>  #define TARGET_F_SETOWN        5       /*  for sockets. */
>  #define TARGET_F_GETOWN        6       /*  for sockets. */
> +#elif defined(TARGET_MIPS)
> +#define TARGET_F_GETLK         14
> +#define TARGET_F_SETLK         6
> +#define TARGET_F_SETLKW        7
> +#define TARGET_F_SETOWN        24       /*  for sockets. */
> +#define TARGET_F_GETOWN        25       /*  for sockets. */
>  #else
>  #define TARGET_F_GETLK         5
>  #define TARGET_F_SETLK         6
> @@ -1673,10 +1679,15 @@ struct target_statfs64 {
>  #define TARGET_F_SETSIG        10      /*  for sockets. */
>  #define TARGET_F_GETSIG        11      /*  for sockets. */
>  
> +#if defined(TARGET_MIPS)
> +#define TARGET_F_GETLK64       33      /*  using 'struct flock64' */
> +#define TARGET_F_SETLK64       34
> +#define TARGET_F_SETLKW64      35
> +#else
>  #define TARGET_F_GETLK64       12      /*  using 'struct flock64' */
>  #define TARGET_F_SETLK64       13
>  #define TARGET_F_SETLKW64      14
> -
> +#endif
>  #if defined (TARGET_ARM)
>  #define TARGET_O_ACCMODE          0003
>  #define TARGET_O_RDONLY             00