[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user.
From: |
Rtp |
Subject: |
Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user. |
Date: |
Wed, 22 Apr 2009 12:04:41 +0200 |
User-agent: |
Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) |
Riku Voipio <address@hidden> 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