From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LwKFQ-0005FX-Fh for qemu-devel@nongnu.org; Tue, 21 Apr 2009 13:59:08 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LwKFO-0005FD-26 for qemu-devel@nongnu.org; Tue, 21 Apr 2009 13:59:07 -0400 Received: from [199.232.76.173] (port=56356 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LwKFN-0005FA-VB for qemu-devel@nongnu.org; Tue, 21 Apr 2009 13:59:05 -0400 Received: from naru.obs2.net ([84.20.150.76]:38186) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LwKFN-00025v-8g for qemu-devel@nongnu.org; Tue, 21 Apr 2009 13:59:05 -0400 Date: Tue, 21 Apr 2009 20:58:58 +0300 From: Riku Voipio Subject: Re: [Qemu-devel] [PATCH] fix fcntl support in linux-user. Message-ID: <20090421175858.GA17579@kos.to> References: <87r5zofk3w.fsf@lechat.rtp-net.org> <20090420132206.GB10865@kos.to> <87d4b6fmct.fsf@lechat.rtp-net.org> <20090421125142.GB15170@kos.to> <20090421143952.GB6375@shareable.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090421143952.GB6375@shareable.org> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jamie Lokier Cc: qemu-devel@nongnu.org, 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).