From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KpUPO-0004T9-Ph for qemu-devel@nongnu.org; Mon, 13 Oct 2008 16:52:54 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KpUPN-0004Sx-9z for qemu-devel@nongnu.org; Mon, 13 Oct 2008 16:52:53 -0400 Received: from [199.232.76.173] (port=46236 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KpUPN-0004Su-50 for qemu-devel@nongnu.org; Mon, 13 Oct 2008 16:52:53 -0400 Received: from hall.aurel32.net ([88.191.82.174]:49733) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KpUPM-0003Ei-He for qemu-devel@nongnu.org; Mon, 13 Oct 2008 16:52:52 -0400 Received: from volta-wlan.aurel32.net ([2002:52e8:2fb:ffff:21d:e0ff:fe49:1047] helo=volta.aurel32.net) by hall.aurel32.net with esmtpsa (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1KpUPK-00039i-KH for qemu-devel@nongnu.org; Mon, 13 Oct 2008 22:52:50 +0200 Received: from aurel32 by volta.aurel32.net with local (Exim 4.69) (envelope-from ) id 1KpUPE-000571-T2 for qemu-devel@nongnu.org; Mon, 13 Oct 2008 22:52:45 +0200 Date: Mon, 13 Oct 2008 22:52:44 +0200 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH] Fix and cleanup IPCOP_msg* ipc calls handling Message-ID: <20081013205244.GA18008@volta.aurel32.net> References: <1223892640-15545-1-git-send-email-kirill@shutemov.name> <1223892640-15545-2-git-send-email-kirill@shutemov.name> <1223892640-15545-3-git-send-email-kirill@shutemov.name> <20081013155311.GA14963@volta.aurel32.net> <20081013184804.GA21226@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <20081013184804.GA21226@localhost.localdomain> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On Mon, Oct 13, 2008 at 09:48:09PM +0300, Kirill A. Shutemov wrote: > On Mon, Oct 13, 2008 at 05:53:11PM +0200, Aurelien Jarno wrote: > > On Mon, Oct 13, 2008 at 01:10:30PM +0300, Kirill A. Shutemov wrote: > > > Signed-off-by: Kirill A. Shutemov > > > --- > > > linux-user/syscall.c | 173 ++++++++++++++++++++++++++++++++++---------------- > > > 1 files changed, 117 insertions(+), 56 deletions(-) > > > > Please find my comments below. In general, please avoid mixing > > indentation changes with code changes. This only makes the code more > > difficult to review. > > Ok. > > By the way, what is correct indentation options for qemu? I configure my vim > with softwidth=4 and set expandtab. Is it correct? I don't really think there is a standard, it seems to vary from file to file. What I do care about is that lines of the patch actually have a code change, not only an indentation change. Otherwise it is difficult to read. > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > > index 40e985a..7e67093 100644 > > > --- a/linux-user/syscall.c > > > +++ b/linux-user/syscall.c > > > @@ -1611,7 +1611,6 @@ static abi_long do_socketcall(int num, abi_ulong vptr) > > > } > > > #endif > > > > > > -#ifdef TARGET_NR_ipc > > > #define N_SHM_REGIONS 32 > > > > > > static struct shm_region { > > > @@ -1845,20 +1844,26 @@ static inline abi_long do_semctl(int first, int second, int third, > > > > > > struct target_msqid_ds > > > { > > > - struct target_ipc_perm msg_perm; > > > - abi_ulong msg_stime; > > > - abi_ulong __unused1; > > > - abi_ulong msg_rtime; > > > - abi_ulong __unused2; > > > - abi_ulong msg_ctime; > > > - abi_ulong __unused3; > > > - abi_ulong __msg_cbytes; > > > - abi_ulong msg_qnum; > > > - abi_ulong msg_qbytes; > > > - abi_ulong msg_lspid; > > > - abi_ulong msg_lrpid; > > > - abi_ulong __unused4; > > > - abi_ulong __unused5; > > > + struct target_ipc_perm msg_perm; > > > + abi_ulong msg_stime; > > > +#if TARGET_ABI_BITS == 32 > > > + abi_ulong __unused1; > > > +#endif > > > + abi_ulong msg_rtime; > > > +#if TARGET_ABI_BITS == 32 > > > + abi_ulong __unused2; > > > +#endif > > > + abi_ulong msg_ctime; > > > +#if TARGET_ABI_BITS == 32 > > > + abi_ulong __unused3; > > > +#endif > > > > Could you explain me why those __unused* are only present with > > TARGET_ABI_BITS? > > They needed to align following filds to 64-bit on 32-bit hosts. Since on > 64-bit hosts sizeof(long) == 8, it's unneeded. > > > This is not consistent with the kernel interface > > defined in the glibc. > > Really? > > glibc 2.5.1, /usr/include/bits/msq.h: This is not the right file to look at, as it may, and actually depends on your architecture. You have to look at the source in the glibc. On my side I was looking at sysdeps/unix/sysv/linux/bits/msq.h, which doesn't have those #ifdef. It is overrided by architectures specific versions. But at the end you are lucky, if you look at all the different msq.h files, your patch is correct. > > > + abi_ulong __msg_cbytes; > > > + abi_ulong msg_qnum; > > > + abi_ulong msg_qbytes; > > > + abi_ulong msg_lspid; > > > + abi_ulong msg_lrpid; > > > + abi_ulong __unused4; > > > + abi_ulong __unused5; > > > }; > > > > > > +static inline abi_long do_msgctl(int msgid, int cmd, abi_long ptr) > > > { > > > struct msqid_ds dsarg; > > > - int cmd = second&0xff; > > > - abi_long ret = 0; > > > - switch( cmd ) { > > > + struct msginfo msginfo; > > > + abi_long ret = -TARGET_EINVAL; > > > + > > > + cmd &= 0xff; > > > + > > > + switch (cmd) { > > > case IPC_STAT: > > > case IPC_SET: > > > - target_to_host_msqid_ds(&dsarg,ptr); > > > - ret = get_errno(msgctl(first, cmd, &dsarg)); > > > - host_to_target_msqid_ds(ptr,&dsarg); > > > - default: > > > - ret = get_errno(msgctl(first, cmd, &dsarg)); > > > > How is default handled now? > > Since we handle all valid cmd, we just return -TARGET_EINVAL. ok. -- .''`. Aurelien Jarno | GPG: 1024D/F1BCDB73 : :' : Debian developer | Electrical Engineer `. `' aurel32@debian.org | aurelien@aurel32.net `- people.debian.org/~aurel32 | www.aurel32.net