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

[Jamie Lokier <jamie@shareable.org>]

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