Re: [Qemu-devel] [linux-user] Added posix message queue syscalls except mq_notify

[Lionel Landwerlin <lionel.landwerlin@openwide.fr>]

Le dimanche 14 décembre 2008 à 20:11 +0200, Kirill A. Shutemov a écrit :
> On Sat, Dec 13, 2008 at 01:39:27PM +0100, Lionel Landwerlin wrote:
> > >From 57a528de47a737e59f391ff7df2f87367b40529e Mon Sep 17 00:00:00 2001
> > From: Lionel Landwerlin <lionel.landwerlin@openwide.fr>
> > Date: Mon, 1 Dec 2008 02:42:24 +0100
> > Subject: [PATCH] Added posix message queue syscalls except mq_notify
> > 
> > Signed-off-by: Lionel Landwerlin <lionel.landwerlin@openwide.fr>
> > 
> > ---
> >  linux-user/syscall.c |  151 ++++++++++++++++++++++++++++++++++++++++++++------
> >  1 files changed, 117 insertions(+), 0 deletions(-)
> > 
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 4065917..c4dd38a 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -28,6 +28,7 @@
> >  #include <fcntl.h>
> >  #include <time.h>
> >  #include <limits.h>
> > +#include <mqueue.h>
> >  #include <sys/types.h>
> >  #include <sys/ipc.h>
> >  #include <sys/msg.h>
> > @@ -629,6 +630,43 @@ static inline abi_long copy_to_user_timeval(abi_ulong target_tv_addr,
> >      return 0;
> >  }
> >  
> > +static inline abi_long copy_from_user_mq_attr(struct mq_attr *attr,
> > +                                              abi_ulong target_mq_attr_addr)
> > +{
> > +    struct mq_attr *target_mq_attr;
> 
> It's wrong. struct mq_attr has long int fields, so you should define
> struct target_mq_attr using abi_long.

I will do that, thx.

> 
> > +
> > +    if (!lock_user_struct(VERIFY_READ, target_mq_attr,
> > +                          target_mq_attr_addr, 1))
> > +        return -TARGET_EFAULT;
> > +
> > +    __get_user(attr->mq_flags, &target_mq_attr->mq_flags);
> > +    __get_user(attr->mq_maxmsg, &target_mq_attr->mq_maxmsg);
> > +    __get_user(attr->mq_msgsize, &target_mq_attr->mq_msgsize);
> > +    __get_user(attr->mq_curmsgs, &target_mq_attr->mq_curmsgs);
> > +
> > +    unlock_user_struct(target_mq_attr, target_mq_attr_addr, 0);
> > +
> > +    return 0;
> > +}
> > +
> > +static inline abi_long copy_to_user_mq_attr(abi_ulong target_mq_attr_addr,
> > +                                            const struct mq_attr *attr)
> > +{
> > +    struct mq_attr *target_mq_attr;
> > +
> > +    if (!lock_user_struct(VERIFY_WRITE, target_mq_attr,
> > +                          target_mq_attr_addr, 0))
> > +        return -TARGET_EFAULT;
> > +
> > +    __put_user(attr->mq_flags, &target_mq_attr->mq_flags);
> > +    __put_user(attr->mq_maxmsg, &target_mq_attr->mq_maxmsg);
> > +    __put_user(attr->mq_msgsize, &target_mq_attr->mq_msgsize);
> > +    __put_user(attr->mq_curmsgs, &target_mq_attr->mq_curmsgs);
> > +
> > +    unlock_user_struct(target_mq_attr, target_mq_attr_addr, 1);
> > +
> > +    return 0;
> > +}
> >  
> >  /* do_select() must return target values and target errnos. */
> >  static abi_long do_select(int n,
> > @@ -6033,6 +6071,85 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
> >          break;
> >  #endif
> >  
> > +#ifdef TARGET_NR_mq_open
> > +    case TARGET_NR_mq_open:
> > +    {
> > +        struct mq_attr posix_mq_attr;
> > +
> > +        p = lock_user_string(arg1 - 1);
> 
> Why - 1?

Look at glibc/uclibc implementation, the string argument is (str + 1).

> 
> > +        if (arg4 != 0)
> > +            copy_from_user_mq_attr (&posix_mq_attr, arg4);
> > +        ret = get_errno(mq_open(p, arg2, arg3, &posix_mq_attr));
> > +        unlock_user (p, arg1, 0);
> > +        break;
> > +    }
> > +
> > +    case TARGET_NR_mq_unlink:
> > +        p = lock_user_string(arg1 - 1);
> 
> ?

Same thing.

> 
> > +        ret = get_errno(mq_unlink(p));
> > +        unlock_user (p, arg1, 0);
> > +        break;
> > +
> > +    case TARGET_NR_mq_timedsend:
> > +    {
> > +        struct timespec ts;
> > +
> > +        if (arg5 != 0) {
> > +            p = lock_user (VERIFY_READ, arg2, arg3, 1);
> > +            target_to_host_timespec(&ts, arg5);
> > +            ret = get_errno(mq_timedsend(arg1, p, arg3, arg4, &ts));
> > +            host_to_target_timespec(arg5, &ts);
> > +            unlock_user (p, arg2, arg3);
> > +        } else {
> > +            p = lock_user (VERIFY_READ, arg2, arg3, 1);
> > +            ret = get_errno(mq_send(arg1, p, arg3, arg4));
> > +            unlock_user (p, arg2, arg3);
> > +        }
> 
> We can lock and unlock outside of if startament, I think.

Right.

> 
> > +        break;
> > +    }
> > +
> > +    case TARGET_NR_mq_timedreceive:
> > +    {
> > +        struct timespec ts;
> > +        unsigned int prio;
> > +
> > +        if (arg5 != 0) {
> > +            p = lock_user (VERIFY_READ, arg2, arg3, 1);
> > +            target_to_host_timespec(&ts, arg5);
> > +            ret = get_errno(mq_timedreceive(arg1, p, arg3, &prio, &ts));
> > +            host_to_target_timespec(arg5, &ts);
> > +            unlock_user (p, arg2, arg3);
> > +        } else {
> > +            p = lock_user (VERIFY_READ, arg2, arg3, 1);
> > +            ret = get_errno(mq_receive(arg1, p, arg3, &prio));
> > +            unlock_user (p, arg2, arg3);
> > +        }
> 
> The same about locking.
> 
> > +        if (arg4 != 0)
> > +            put_user_u32(prio, arg4);
> > +        break;
> > +    }
> > +
> > +    /* Not implemented for now... */
> > +/*     case TARGET_NR_mq_notify: */
> > +/*         break; */
> 
> Is there any problem with this syscall?

This syscall is a little bit more complicated. The implementation would
be a kind of signal handler.

> 
> > +
> > +    case TARGET_NR_mq_getsetattr:
> > +    {
> > +        struct mq_attr posix_mq_attr_in, posix_mq_attr_out;
> > +
> > +        if (arg3 != 0) {
> > +            ret = mq_getattr(arg1, &posix_mq_attr_out);
> > +            copy_to_user_mq_attr(arg3, &posix_mq_attr_out);
> > +        }
> > +        if (arg2 != 0) {
> > +            copy_from_user_mq_attr(&posix_mq_attr_in, arg2);
> > +            ret |= mq_setattr(arg1, &posix_mq_attr_in, &posix_mq_attr_out);
> > +        }
> > +
> > +        break;
> > +    }
> > +#endif
> > +
> >      default:
> >      unimplemented:
> >          gemu_log("qemu: Unsupported syscall: %d\n", num);
> > -- 
> > 1.5.6.5