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

[Arnaud Patard (Rtp) <arnaud.patard@rtp-net.org>]

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