Re: [Qemu-devel] [PATCH] linux-user: fix convertion of flock/flock64 l_type field

[Max Filippov]

On Tue, May 8, 2018 at 9:36 AM, Laurent Vivier <address@hidden> wrote:
> As l_type values (F_RDLCK, F_WRLCK, F_UNLCK, F_EXLCK, F_SHLCK)
> are not bitmasks, we can't use target_to_host_bitmask() and
> host_to_target_bitmask() to convert them.
>
> Introduce target_to_host_flock() and host_to_target_flock()
> to convert values between host and target.
>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  linux-user/syscall.c | 48 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e4825747f9..fdf6766eca 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6546,15 +6546,33 @@ static int target_to_host_fcntl_cmd(int cmd)
>      return -TARGET_EINVAL;
>  }
>
> -#define TRANSTBL_CONVERT(a) { -1, TARGET_##a, -1, a }
> -static const bitmask_transtbl flock_tbl[] = {
> -    TRANSTBL_CONVERT(F_RDLCK),
> -    TRANSTBL_CONVERT(F_WRLCK),
> -    TRANSTBL_CONVERT(F_UNLCK),
> -    TRANSTBL_CONVERT(F_EXLCK),
> -    TRANSTBL_CONVERT(F_SHLCK),
> -    { 0, 0, 0, 0 }
> -};
> +static unsigned int target_to_host_flock(unsigned int type)
> +{
> +    switch (type) {
> +#define TRANSTBL_CONVERT(a) case TARGET_##a: return a;
> +    TRANSTBL_CONVERT(F_RDLCK)
> +    TRANSTBL_CONVERT(F_WRLCK)
> +    TRANSTBL_CONVERT(F_UNLCK)
> +    TRANSTBL_CONVERT(F_EXLCK)
> +    TRANSTBL_CONVERT(F_SHLCK)
> +#undef  TRANSTBL_CONVERT
> +    }
> +    return type;

Shouldn't it return something special, like -1 in case of conversion failure?

> +}
> +
> +static unsigned int host_to_target_flock(unsigned int type)
> +{
> +    switch (type) {
> +#define TRANSTBL_CONVERT(a) case a: return TARGET_##a;
> +    TRANSTBL_CONVERT(F_RDLCK)
> +    TRANSTBL_CONVERT(F_WRLCK)
> +    TRANSTBL_CONVERT(F_UNLCK)
> +    TRANSTBL_CONVERT(F_EXLCK)
> +    TRANSTBL_CONVERT(F_SHLCK)
> +#undef  TRANSTBL_CONVERT
> +    }
> +    return type;
> +}
>

There's a duplication. Wouldn't it be better if it was done like the following:

#define FLOCK_TRANSTBL \
    switch (type) {
    TRANSTBL_CONVERT(F_RDLCK) \
    TRANSTBL_CONVERT(F_WRLCK) \
    TRANSTBL_CONVERT(F_UNLCK) \
    TRANSTBL_CONVERT(F_EXLCK) \
    TRANSTBL_CONVERT(F_SHLCK) \
    }

static unsigned int target_to_host_flock(unsigned int type)
{
#define TRANSTBL_CONVERT(a) case TARGET_##a: return a;
    FLOCK_TRANSTBL
#undef  TRANSTBL_CONVERT
    return -1;
}

static unsigned int host_to_target_flock(unsigned int type)
{
#define TRANSTBL_CONVERT(a) case a: return TARGET_##a;
    FLOCK_TRANSTBL
#undef  TRANSTBL_CONVERT
    return -1;
}

#undef FLOCK_TRANSTBL

-- 
Thanks.
-- Max