Re: [Qemu-devel] [PATCH] Fix and cleanup IPCOP_msg* ipc calls handling

["Kirill A. Shutemov" <kirill@shutemov.name>]

[-- Attachment #1: Type: text/plain, Size: 4343 bytes --]

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 <kirill@shutemov.name>
> > ---
> >  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?

> > 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:
 38 struct msqid_ds
 39 {
 40   struct ipc_perm msg_perm;     /* structure describing operation permi
 41   __time_t msg_stime;           /* time of last msgsnd command */
 42 #if __WORDSIZE == 32
 43   unsigned long int __unused1;
 44 #endif
 45   __time_t msg_rtime;           /* time of last msgrcv command */
 46 #if __WORDSIZE == 32
 47   unsigned long int __unused2;
 48 #endif
 49   __time_t msg_ctime;           /* time of last change */
 50 #if __WORDSIZE == 32
 51   unsigned long int __unused3;
 52 #endif
 53   unsigned long int __msg_cbytes; /* current number of bytes on queue *
 54   msgqnum_t msg_qnum;           /* number of messages currently on queu
 55   msglen_t msg_qbytes;          /* max number of bytes allowed on queue
 56   __pid_t msg_lspid;            /* pid of last msgsnd() */
 57   __pid_t msg_lrpid;            /* pid of last msgrcv() */
 58   unsigned long int __unused4;
 59   unsigned long int __unused5;
 60 };

> > +    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;
> >  };

<skip/>  

> > +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.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]