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

[Riku Voipio]

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