| Message ID | 20180508163639.7138-1-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user: fix convertion of flock/flock64 l_type field | expand |
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
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 ';').
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
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);
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(-)