* [Qemu-devel] [PATCH] fix fcntl support in linux-user. @ 2009-04-19 20:45 Arnaud Patard 2009-04-20 13:22 ` Riku Voipio 0 siblings, 1 reply; 7+ messages in thread From: Arnaud Patard @ 2009-04-19 20:45 UTC (permalink / raw) To: qemu-devel [-- Attachment #1: Type: text/plain, Size: 535 bytes --] The current code in do_fcntl is passing the target command as a host command. This is introducing 2 problems: - When building with "-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE", we may end up passing commands like F_GETFL instead of F_GETFL64. This is likely to return an error. - The F_*64 constants doesn't always have the same value between host and target. Without this patch locking is not working (My test-case was pwck with arm as target and mips as host). Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org> --- [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: fix_fcntl.patch --] [-- Type: text/x-diff, Size: 2148 bytes --] diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 2d51d6b..2d876c1 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -3126,7 +3141,7 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) 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, F_GETLK, &fl)); if (ret == 0) { if (!lock_user_struct(VERIFY_WRITE, target_fl, arg, 0)) return -TARGET_EFAULT; @@ -3149,7 +3164,7 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) 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, F_SETLK+(cmd-TARGET_F_SETLK), &fl)); break; case TARGET_F_GETLK64: @@ -3161,7 +3176,7 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) 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, F_GETLK64, &fl64)); if (ret == 0) { if (!lock_user_struct(VERIFY_WRITE, target_fl64, arg, 0)) return -TARGET_EFAULT; @@ -3183,7 +3198,7 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) 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, F_SETLK64+(cmd-TARGET_F_SETLK64), &fl64)); break; case F_GETFL: @@ -6022,7 +6039,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, ret = get_errno(fcntl(arg1, cmd, &fl)); break; default: - ret = do_fcntl(arg1, cmd, arg3); + ret = do_fcntl(arg1, arg2, arg3); break; } break; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user. 2009-04-19 20:45 [Qemu-devel] [PATCH] fix fcntl support in linux-user Arnaud Patard @ 2009-04-20 13:22 ` Riku Voipio 2009-04-21 8:21 ` Arnaud Patard 0 siblings, 1 reply; 7+ messages in thread From: Riku Voipio @ 2009-04-20 13:22 UTC (permalink / raw) To: qemu-devel; +Cc: arnaud.patard On Sun, Apr 19, 2009 at 10:45:39PM +0200, Arnaud Patard wrote: > > The current code in do_fcntl is passing the target command as a host > command. This is introducing 2 problems: > > - When building with "-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE", we > may end up passing commands like F_GETFL instead of F_GETFL64. This is > likely to return an error. > > - The F_*64 constants doesn't always have the same value between host > and target. This appears to be the case for some other fcntl constants as well. Howabout implementing this as a target_to_host_fcntl_cmd() mapping function instead? > Without this patch locking is not working (My test-case was pwck with arm as > target and mips as host). Could you also add the MIPS target definition for F_*64 so that mips on arm would get fixed too? > Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org> > --- > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 2d51d6b..2d876c1 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -3126,7 +3141,7 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) > 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, F_GETLK, &fl)); > if (ret == 0) { > if (!lock_user_struct(VERIFY_WRITE, target_fl, arg, 0)) > return -TARGET_EFAULT; > @@ -3149,7 +3164,7 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) > 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, F_SETLK+(cmd-TARGET_F_SETLK), &fl)); > break; > > case TARGET_F_GETLK64: > @@ -3161,7 +3176,7 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) > 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, F_GETLK64, &fl64)); > if (ret == 0) { > if (!lock_user_struct(VERIFY_WRITE, target_fl64, arg, 0)) > return -TARGET_EFAULT; > @@ -3183,7 +3198,7 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) > 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, F_SETLK64+(cmd-TARGET_F_SETLK64), &fl64)); > break; > > case F_GETFL: > @@ -6022,7 +6039,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > ret = get_errno(fcntl(arg1, cmd, &fl)); > break; > default: > - ret = do_fcntl(arg1, cmd, arg3); > + ret = do_fcntl(arg1, arg2, arg3); > break; > } > break; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user. 2009-04-20 13:22 ` Riku Voipio @ 2009-04-21 8:21 ` Arnaud Patard 2009-04-21 12:51 ` Riku Voipio 0 siblings, 1 reply; 7+ messages in thread From: Arnaud Patard @ 2009-04-21 8:21 UTC (permalink / raw) To: Riku Voipio; +Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 977 bytes --] Riku Voipio <riku.voipio@iki.fi> writes: Hi, > On Sun, Apr 19, 2009 at 10:45:39PM +0200, Arnaud Patard wrote: >> >> The current code in do_fcntl is passing the target command as a host >> command. This is introducing 2 problems: >> >> - When building with "-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE", we >> may end up passing commands like F_GETFL instead of F_GETFL64. This is >> likely to return an error. >> >> - The F_*64 constants doesn't always have the same value between host >> and target. > > This appears to be the case for some other fcntl constants as well. Howabout > implementing this as a target_to_host_fcntl_cmd() mapping function instead? Ok, done. New patch attached. Please comment :) > >> Without this patch locking is not working (My test-case was pwck with arm as >> target and mips as host). > > Could you also add the MIPS target definition for F_*64 so that mips on arm > would get fixed too? oops. Forgot to include that in my patch [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: fix_fcntl-2.patch --] [-- Type: text/x-diff, Size: 5432 bytes --] 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, +}; #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]; 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user. 2009-04-21 8:21 ` Arnaud Patard @ 2009-04-21 12:51 ` Riku Voipio 2009-04-21 14:39 ` Jamie Lokier 0 siblings, 1 reply; 7+ messages in thread From: Riku Voipio @ 2009-04-21 12:51 UTC (permalink / raw) To: Arnaud Patard; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user. 2009-04-21 12:51 ` Riku Voipio @ 2009-04-21 14:39 ` Jamie Lokier 2009-04-21 17:58 ` Riku Voipio 0 siblings, 1 reply; 7+ messages in thread From: Jamie Lokier @ 2009-04-21 14:39 UTC (permalink / raw) To: Riku Voipio; +Cc: qemu-devel, Arnaud Patard Riku Voipio wrote: > 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. > > > +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? And check for unset entries. The new code doesn't return EINVAL for unknown commands as it should. (But it calls the host with a zero command _if_ the target command is smaller than the table... which results in EINVAL). The old code wasn't perfect, passing junk command values to the host that it didn't recognise. Also, are TARGET_F_GETLK64 and TARGET_F_GETLK distinct values on 64-bit hosts - or even exist at all? > cmd2 is not a very good variable name, for example host_cmd > would describe it better. Agree. > > - 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]; The new code behaves differently for unknown arg2 values. The old code passed junk to the host kernel; the new code passes zero if arg2 < the table size, and reads outside the array otherwise. Both are surely wrong? Simply return EINVAL if arg2 isn't recognised. I'm inclined to keeping the switch, adding the other cases (TARGET_F_GETLK etc.), #ifdef around the ..64 ones, and making the default case return EINVAL explicitly. A table lookup wouldn't save anything once you've checked its bounds and for the no-entry case. room. -- Jamie ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user. 2009-04-21 14:39 ` Jamie Lokier @ 2009-04-21 17:58 ` Riku Voipio 2009-04-22 10:04 ` Arnaud Patard 0 siblings, 1 reply; 7+ messages in thread From: Riku Voipio @ 2009-04-21 17:58 UTC (permalink / raw) To: Jamie Lokier; +Cc: qemu-devel, Arnaud Patard On Tue, Apr 21, 2009 at 03:39:52PM +0100, Jamie Lokier wrote: > > explict size for table and check for not overflowing? > And check for unset entries. The new code doesn't return EINVAL for > unknown commands as it should. (But it calls the host with a zero > command _if_ the target command is smaller than the table... which > results in EINVAL). The old code wasn't perfect, passing junk command > values to the host that it didn't recognise. > Also, are TARGET_F_GETLK64 and TARGET_F_GETLK distinct values on > 64-bit hosts - or even exist at all? The old code appears to be a slightly incoherant mix of fcntl, fcntl64, GETLK64, GETLK, ... and in a need of major cleanup, too. > > > - 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]; > The new code behaves differently for unknown arg2 values. The old > code passed junk to the host kernel; the new code passes zero if arg2 > < the table size, and reads outside the array otherwise. Both are > surely wrong? Simply return EINVAL if arg2 isn't recognised. note that that was just the fcntl64 syscall, the other place was fcntl. > I'm inclined to keeping the switch, adding the other cases > (TARGET_F_GETLK etc.), #ifdef around the ..64 ones, and making the > default case return EINVAL explicitly. A table lookup wouldn't save > anything once you've checked its bounds and for the no-entry case. > room. My original idea was that a mapping function would make it clearer what is being done, and the main save being able to use it from two places (fcntl and fcntl64). ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user. 2009-04-21 17:58 ` Riku Voipio @ 2009-04-22 10:04 ` Arnaud Patard 0 siblings, 0 replies; 7+ messages in thread From: Arnaud Patard @ 2009-04-22 10:04 UTC (permalink / raw) To: Riku Voipio; +Cc: qemu-devel Riku Voipio <riku.voipio@iki.fi> writes: > On Tue, Apr 21, 2009 at 03:39:52PM +0100, Jamie Lokier wrote: >> > explict size for table and check for not overflowing? > >> And check for unset entries. The new code doesn't return EINVAL for >> unknown commands as it should. (But it calls the host with a zero >> command _if_ the target command is smaller than the table... which >> results in EINVAL). The old code wasn't perfect, passing junk command >> values to the host that it didn't recognise. > >> Also, are TARGET_F_GETLK64 and TARGET_F_GETLK distinct values on >> 64-bit hosts - or even exist at all? > > The old code appears to be a slightly incoherant mix of fcntl, fcntl64, > GETLK64, GETLK, ... and in a need of major cleanup, too. I would rather say that it's doing what the kernel is doing :) Look at fs/fcntl.c and you'll see a very similar code. Having similar code between qemu and kernel for the linux-user syscall stuff is a good idea as it makes sure the emulated syscall has a very similar behaviour as the real one. > >> > > - 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]; > >> The new code behaves differently for unknown arg2 values. The old >> code passed junk to the host kernel; the new code passes zero if arg2 >> < the table size, and reads outside the array otherwise. Both are >> surely wrong? Simply return EINVAL if arg2 isn't recognised. > > note that that was just the fcntl64 syscall, the other place was fcntl. > >> I'm inclined to keeping the switch, adding the other cases >> (TARGET_F_GETLK etc.), #ifdef around the ..64 ones, and making the >> default case return EINVAL explicitly. A table lookup wouldn't save >> anything once you've checked its bounds and for the no-entry case. >> room. keeping the first fcntl64 switch is making things harder to understand. Also, the do_fcntl call should be kept to avoid duplicating code imho. > > My original idea was that a mapping function would make it clearer > what is being done, and the main save being able to use it from > two places (fcntl and fcntl64). I though it was better to use an array instead of a function with something like a switch() but looks like I can't avoid making one. Will wait for new comments (if there are some) and come with a new patch tomorrow. Thanks, Arnaud ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-04-22 9:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-04-19 20:45 [Qemu-devel] [PATCH] fix fcntl support in linux-user Arnaud Patard 2009-04-20 13:22 ` Riku Voipio 2009-04-21 8:21 ` Arnaud Patard 2009-04-21 12:51 ` Riku Voipio 2009-04-21 14:39 ` Jamie Lokier 2009-04-21 17:58 ` Riku Voipio 2009-04-22 10:04 ` Arnaud Patard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).