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

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

[Laurent Vivier]

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 <laurent@vivier.eu>
---
 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;
+}
+
+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;
+}
 
 static inline abi_long copy_from_user_flock(struct flock64 *fl,
                                             abi_ulong target_flock_addr)
@@ -6567,7 +6585,7 @@ static inline abi_long copy_from_user_flock(struct flock64 *fl,
     }
 
     __get_user(l_type, &target_fl->l_type);
-    fl->l_type = target_to_host_bitmask(l_type, flock_tbl);
+    fl->l_type = target_to_host_flock(l_type);
     __get_user(fl->l_whence, &target_fl->l_whence);
     __get_user(fl->l_start, &target_fl->l_start);
     __get_user(fl->l_len, &target_fl->l_len);
@@ -6586,7 +6604,7 @@ static inline abi_long copy_to_user_flock(abi_ulong target_flock_addr,
         return -TARGET_EFAULT;
     }
 
-    l_type = host_to_target_bitmask(fl->l_type, flock_tbl);
+    l_type = host_to_target_flock(fl->l_type);
     __put_user(l_type, &target_fl->l_type);
     __put_user(fl->l_whence, &target_fl->l_whence);
     __put_user(fl->l_start, &target_fl->l_start);
@@ -6611,7 +6629,7 @@ static inline abi_long copy_from_user_oabi_flock64(struct flock64 *fl,
     }
 
     __get_user(l_type, &target_fl->l_type);
-    fl->l_type = target_to_host_bitmask(l_type, flock_tbl);
+    fl->l_type = target_to_host_flock(l_type);
     __get_user(fl->l_whence, &target_fl->l_whence);
     __get_user(fl->l_start, &target_fl->l_start);
     __get_user(fl->l_len, &target_fl->l_len);
@@ -6630,7 +6648,7 @@ static inline abi_long copy_to_user_oabi_flock64(abi_ulong target_flock_addr,
         return -TARGET_EFAULT;
     }
 
-    l_type = host_to_target_bitmask(fl->l_type, flock_tbl);
+    l_type = host_to_target_flock(fl->l_type);
     __put_user(l_type, &target_fl->l_type);
     __put_user(fl->l_whence, &target_fl->l_whence);
     __put_user(fl->l_start, &target_fl->l_start);
@@ -6652,7 +6670,7 @@ static inline abi_long copy_from_user_flock64(struct flock64 *fl,
     }
 
     __get_user(l_type, &target_fl->l_type);
-    fl->l_type = target_to_host_bitmask(l_type, flock_tbl);
+    fl->l_type = target_to_host_flock(l_type);
     __get_user(fl->l_whence, &target_fl->l_whence);
     __get_user(fl->l_start, &target_fl->l_start);
     __get_user(fl->l_len, &target_fl->l_len);
@@ -6671,7 +6689,7 @@ static inline abi_long copy_to_user_flock64(abi_ulong target_flock_addr,
         return -TARGET_EFAULT;
     }
 
-    l_type = host_to_target_bitmask(fl->l_type, flock_tbl);
+    l_type = host_to_target_flock(fl->l_type);
     __put_user(l_type, &target_fl->l_type);
     __put_user(fl->l_whence, &target_fl->l_whence);
     __put_user(fl->l_start, &target_fl->l_start);
-- 
2.14.3

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 <laurent@vivier.eu> 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 <laurent@vivier.eu>
> ---
>  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

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

[Eric Blake]

On 05/08/2018 12:08 PM, Max Filippov wrote:
> On Tue, May 8, 2018 at 9:36 AM, Laurent Vivier <laurent@vivier.eu> wrote:

In the subject, s/convertion/conversion/

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

>> +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) \

If you do this, I'd lean towards omitting the trailing ; from 
TRANSTBL_CONVERT() and sticking it in FLOCK_TRANSTBL instead (it looks 
weird to see a statement-like macro called without a ';').

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Laurent Vivier]

Le 08/05/2018 à 20:55, Eric Blake a écrit :
> On 05/08/2018 12:08 PM, Max Filippov wrote:
>> On Tue, May 8, 2018 at 9:36 AM, Laurent Vivier <laurent@vivier.eu> wrote:
> 
> In the subject, s/convertion/conversion/
> 
>>> 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.
>>>
> 
>>> +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) \
> 
> If you do this, I'd lean towards omitting the trailing ; from
> TRANSTBL_CONVERT() and sticking it in FLOCK_TRANSTBL instead (it looks
> weird to see a statement-like macro called without a ';').
> 

I have included this patch in the series "linux-user: fix sparc32plus"
and missed your comments. I'll update this patch in the v2 of the series.

Thanks,
Laurent