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

[Aurelien Jarno]

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 <address@hidden>
> > > ---
> > >  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;
> > >  };
> 
> <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.

ok.

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net