[Qemu-devel] [PATCH v2 0/7] Improvements of qemu linux-user

[Qemu-devel] [PATCH v2 0/7] Improvements of qemu linux-user

[Lena Djokic]

v2: added 6 patches
This patch series contains implementation of support for 
two new system calls, and fixes for 5 existing system calls,
and fix for a structure definition as well.

Lena Djokic (7):
  linux-user: Add fanotify implementation
  linux-user: Fix inotify_init1 support
  linux-user: Fix flock definition for mips64
  linux-user: Fix fcnt
  linux-user: Fix readahead
  linux-user: Fix syslog
  linux-user: Fix mq_open

 configure                 |  20 ++++++
 linux-user/syscall.c      | 166 ++++++++++++++++++++++++++++++++++++++++------
 linux-user/syscall_defs.h |   2 +-
 3 files changed, 168 insertions(+), 20 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/7] linux-user: Add fanotify implementation

[Lena Djokic]

This commit adds implementation of fanotify_init and fanotify_mark.
Second argument for fanotify_init needs conversion because of flags
which can be FAN_NONBLOCK and FAN_CLOEXEC which rely on O_NONBLOCK
and O_CLOEXEC and those can have different values on different platforms.
For fanotify_mark argument layout is different for 32-bit and 64-bit
platforms and this implementation have support for that situation.
Also, support for writing and reading of file descriptor opened by
fanotify_init is added.
Configure file contains checks for excistence of fanotify support on
given build system.

Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
---
 configure            |  20 ++++++++
 linux-user/syscall.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 142 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index fd6f898..56e6c98 100755
--- a/configure
+++ b/configure
@@ -3537,6 +3537,23 @@ if compile_prog "" "" ; then
   inotify1=yes
 fi
 
+# check if fanotify group of system calls is supported
+fanotify=no
+cat > $TMPC << EOF
+#include <sys/fanotify.h>
+
+int
+main(void)
+{
+    fanotify_init(0,0);
+    fanotify_mark(0,0,0,0,0);
+    return 0;
+}
+EOF
+if compile_prog "" "" ; then
+  fanotify=yes
+fi
+
 # check if utimensat and futimens are supported
 utimens=no
 cat > $TMPC << EOF
@@ -5335,6 +5352,9 @@ fi
 if test "$inotify1" = "yes" ; then
   echo "CONFIG_INOTIFY1=y" >> $config_host_mak
 fi
+if test "$fanotify" = "yes" ; then
+  echo "CONFIG_FANOTIFY=y" >> $config_host_mak
+fi
 if test "$byteswap_h" = "yes" ; then
   echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak
 fi
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 7b77503..f5d9a26 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -76,6 +76,9 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 #ifdef CONFIG_SENDFILE
 #include <sys/sendfile.h>
 #endif
+#ifdef CONFIG_FANOTIFY
+#include <sys/fanotify.h>
+#endif
 
 #define termios host_termios
 #define winsize host_winsize
@@ -499,9 +502,13 @@ enum {
     QEMU___IFLA_INET6_MAX
 };
 
+typedef abi_long (*TargetFdReadFunc)(void *, size_t);
+typedef abi_long (*TargetFdWriteFunc)(void *, size_t);
 typedef abi_long (*TargetFdDataFunc)(void *, size_t);
 typedef abi_long (*TargetFdAddrFunc)(void *, abi_ulong, socklen_t);
 typedef struct TargetFdTrans {
+    TargetFdReadFunc read_op;
+    TargetFdWriteFunc write_op;
     TargetFdDataFunc host_to_target_data;
     TargetFdDataFunc target_to_host_data;
     TargetFdAddrFunc target_to_host_addr;
@@ -511,6 +518,22 @@ static TargetFdTrans **target_fd_trans;
 
 static unsigned int target_fd_max;
 
+static TargetFdReadFunc fd_trans_read_op(int fd)
+{
+    if (fd >= 0 && fd < target_fd_max && target_fd_trans[fd]) {
+        return target_fd_trans[fd]->read_op;
+    }
+    return NULL;
+}
+
+static TargetFdWriteFunc fd_trans_write_op(int fd)
+{
+    if (fd >= 0 && fd < target_fd_max && target_fd_trans[fd]) {
+        return target_fd_trans[fd]->write_op;
+    }
+    return NULL;
+}
+
 static TargetFdDataFunc fd_trans_target_to_host_data(int fd)
 {
     if (fd >= 0 && fd < target_fd_max && target_fd_trans[fd]) {
@@ -7527,6 +7550,47 @@ static target_timer_t get_timer_id(abi_long arg)
     return timerid;
 }
 
+#if defined(CONFIG_FANOTIFY)
+static inline abi_long fanotify_fd_read_op(void *buf, size_t len)
+{
+    struct fanotify_event_metadata *fem;
+    int num;
+
+    /* Read buffer for fanotify file descriptor contains one or more
+     * of fanotify_event_metadata structures.
+     */
+    fem = (struct fanotify_event_metadata *)buf;
+    num = len / sizeof(struct fanotify_event_metadata);
+    for (int i = 0; i < num; i++) {
+        (fem + i)->event_len = tswap32((fem + i)->event_len);
+        /* Fields (fem+i)->vers and (fem+i)->reserved are single byte,
+         * so swapping is not needed for them.
+         */
+        (fem + i)->metadata_len = tswap16((fem + i)->metadata_len);
+        (fem + i)->mask = tswap64((fem + i)->mask);
+        (fem + i)->fd = tswap32((fem + i)->fd);
+        (fem + i)->pid = tswap32((fem + i)->pid);
+    }
+
+    return len;
+}
+
+static inline abi_long fanotify_fd_write_op(void *buf, size_t len)
+{
+    struct fanotify_response *fr = (struct fanotify_response *)buf;
+
+    fr->fd = tswap32(fr->fd);
+    fr->response = tswap32(fr->response);
+
+    return len;
+}
+
+static TargetFdTrans fanotify_trans = {
+    .read_op = fanotify_fd_read_op,
+    .write_op = fanotify_fd_write_op,
+};
+#endif
+
 /* do_syscall() should always have a single exit point at the end so
    that actions, such as logging of syscall results, can be performed.
    All errnos that do_syscall() returns must be -TARGET_<errcode>. */
@@ -7613,16 +7677,27 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             if (!(p = lock_user(VERIFY_WRITE, arg2, arg3, 0)))
                 goto efault;
             ret = get_errno(safe_read(arg1, p, arg3));
-            if (ret >= 0 &&
-                fd_trans_host_to_target_data(arg1)) {
-                ret = fd_trans_host_to_target_data(arg1)(p, ret);
-            }
+            if (ret >= 0) {
+                if (fd_trans_read_op(arg1)) {
+                    ret = fd_trans_read_op(arg1)(p, ret);
+                }
+                if (fd_trans_host_to_target_data(arg1)) {
+                    ret = fd_trans_host_to_target_data(arg1)(p, ret);
+                }
+             }
             unlock_user(p, arg2, ret);
         }
         break;
     case TARGET_NR_write:
         if (!(p = lock_user(VERIFY_READ, arg2, arg3, 1)))
             goto efault;
+        if (fd_trans_write_op(arg1)) {
+            ret = fd_trans_write_op(arg1)(p, arg3);
+            if (is_error(ret)) {
+                unlock_user(p, arg2, 0);
+                break;
+            }
+        }
         ret = get_errno(safe_write(arg1, p, arg3));
         unlock_user(p, arg2, 0);
         break;
@@ -11567,6 +11642,49 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         break;
 #endif
 
+#if defined(TARGET_NR_fanotify_init) && defined(CONFIG_FANOTIFY)
+    case TARGET_NR_fanotify_init:
+        {
+            ret = get_errno(fanotify_init(arg1, target_to_host_bitmask(arg2,
+                                                fcntl_flags_tbl)));
+            if (ret >= 0) {
+                fd_trans_register(ret, &fanotify_trans);
+            }
+        }
+        break;
+#endif
+#if defined(TARGET_NR_fanotify_mark) && defined(CONFIG_FANOTIFY)
+    case TARGET_NR_fanotify_mark:
+        {
+            p = NULL;
+#if (TARGET_ABI_BITS == 32)
+            if (arg6) {
+                p = lock_user_string(arg6);
+                if (!p) {
+                    goto efault;
+                }
+            }
+            ret = get_errno(fanotify_mark(arg1, arg2,
+                                target_offset64(arg3, arg4), arg5 , p));
+            if (arg6) {
+                unlock_user(p, arg6, 0);
+            }
+#else
+            if (arg5) {
+                p = lock_user_string(arg5);
+                if (!p) {
+                    goto efault;
+                }
+            }
+            ret = get_errno(fanotify_mark(arg1, arg2, arg3, arg4 , p));
+            if (arg5) {
+                unlock_user(p, arg5, 0);
+            }
+#endif
+        }
+        break;
+#endif
+
 #if defined(TARGET_NR_mq_open) && defined(__NR_mq_open)
     case TARGET_NR_mq_open:
         {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/7] linux-user: Add fanotify implementation

[Peter Maydell]

On 24 November 2016 at 16:08, Lena Djokic <Lena.Djokic@rt-rk.com> wrote:
> This commit adds implementation of fanotify_init and fanotify_mark.
> Second argument for fanotify_init needs conversion because of flags
> which can be FAN_NONBLOCK and FAN_CLOEXEC which rely on O_NONBLOCK
> and O_CLOEXEC and those can have different values on different platforms.
> For fanotify_mark argument layout is different for 32-bit and 64-bit
> platforms and this implementation have support for that situation.
> Also, support for writing and reading of file descriptor opened by
> fanotify_init is added.
> Configure file contains checks for excistence of fanotify support on
> given build system.
>
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>

Thanks for this patch; it looks basically the right shape
but I have some review comments below. (Also, sorry for taking
so long to get to reviewing it.)

> ---

> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 7b77503..f5d9a26 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -76,6 +76,9 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
>  #ifdef CONFIG_SENDFILE
>  #include <sys/sendfile.h>
>  #endif
> +#ifdef CONFIG_FANOTIFY
> +#include <sys/fanotify.h>
> +#endif
>
>  #define termios host_termios
>  #define winsize host_winsize
> @@ -499,9 +502,13 @@ enum {
>      QEMU___IFLA_INET6_MAX
>  };
>
> +typedef abi_long (*TargetFdReadFunc)(void *, size_t);
> +typedef abi_long (*TargetFdWriteFunc)(void *, size_t);
>  typedef abi_long (*TargetFdDataFunc)(void *, size_t);
>  typedef abi_long (*TargetFdAddrFunc)(void *, abi_ulong, socklen_t);
>  typedef struct TargetFdTrans {
> +    TargetFdReadFunc read_op;
> +    TargetFdWriteFunc write_op;
>      TargetFdDataFunc host_to_target_data;
>      TargetFdDataFunc target_to_host_data;
>      TargetFdAddrFunc target_to_host_addr;

What's the difference between read_op/write_op and the existing
target_to_host_data/host_to_target_data hooks ? I feel like we
should just use the existing hooks unless there's something
specific that means they don't work. If we do need extra hooks
we should add doc comments so it's clear which hooks get
invoked in which contexts.


> +#if defined(CONFIG_FANOTIFY)
> +static inline abi_long fanotify_fd_read_op(void *buf, size_t len)
> +{
> +    struct fanotify_event_metadata *fem;
> +    int num;

This should probably be a size_t, or you have problems with
really large reads.

> +
> +    /* Read buffer for fanotify file descriptor contains one or more
> +     * of fanotify_event_metadata structures.
> +     */
> +    fem = (struct fanotify_event_metadata *)buf;
> +    num = len / sizeof(struct fanotify_event_metadata);
> +    for (int i = 0; i < num; i++) {
> +        (fem + i)->event_len = tswap32((fem + i)->event_len);
> +        /* Fields (fem+i)->vers and (fem+i)->reserved are single byte,
> +         * so swapping is not needed for them.
> +         */
> +        (fem + i)->metadata_len = tswap16((fem + i)->metadata_len);
> +        (fem + i)->mask = tswap64((fem + i)->mask);
> +        (fem + i)->fd = tswap32((fem + i)->fd);
> +        (fem + i)->pid = tswap32((fem + i)->pid);
> +    }
> +
> +    return len;
> +}
> +
> +static inline abi_long fanotify_fd_write_op(void *buf, size_t len)
> +{
> +    struct fanotify_response *fr = (struct fanotify_response *)buf;
> +
> +    fr->fd = tswap32(fr->fd);
> +    fr->response = tswap32(fr->response);
> +
> +    return len;
> +}
> +
> +static TargetFdTrans fanotify_trans = {
> +    .read_op = fanotify_fd_read_op,
> +    .write_op = fanotify_fd_write_op,
> +};
> +#endif
> +
>  /* do_syscall() should always have a single exit point at the end so
>     that actions, such as logging of syscall results, can be performed.
>     All errnos that do_syscall() returns must be -TARGET_<errcode>. */
> @@ -7613,16 +7677,27 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              if (!(p = lock_user(VERIFY_WRITE, arg2, arg3, 0)))
>                  goto efault;
>              ret = get_errno(safe_read(arg1, p, arg3));
> -            if (ret >= 0 &&
> -                fd_trans_host_to_target_data(arg1)) {
> -                ret = fd_trans_host_to_target_data(arg1)(p, ret);
> -            }
> +            if (ret >= 0) {
> +                if (fd_trans_read_op(arg1)) {
> +                    ret = fd_trans_read_op(arg1)(p, ret);
> +                }
> +                if (fd_trans_host_to_target_data(arg1)) {
> +                    ret = fd_trans_host_to_target_data(arg1)(p, ret);
> +                }
> +             }
>              unlock_user(p, arg2, ret);
>          }
>          break;
>      case TARGET_NR_write:
>          if (!(p = lock_user(VERIFY_READ, arg2, arg3, 1)))
>              goto efault;
> +        if (fd_trans_write_op(arg1)) {
> +            ret = fd_trans_write_op(arg1)(p, arg3);
> +            if (is_error(ret)) {
> +                unlock_user(p, arg2, 0);
> +                break;
> +            }
> +        }
>          ret = get_errno(safe_write(arg1, p, arg3));
>          unlock_user(p, arg2, 0);
>          break;
> @@ -11567,6 +11642,49 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          break;
>  #endif
>
> +#if defined(TARGET_NR_fanotify_init) && defined(CONFIG_FANOTIFY)
> +    case TARGET_NR_fanotify_init:
> +        {
> +            ret = get_errno(fanotify_init(arg1, target_to_host_bitmask(arg2,
> +                                                fcntl_flags_tbl)));
> +            if (ret >= 0) {
> +                fd_trans_register(ret, &fanotify_trans);
> +            }
> +        }
> +        break;
> +#endif
> +#if defined(TARGET_NR_fanotify_mark) && defined(CONFIG_FANOTIFY)
> +    case TARGET_NR_fanotify_mark:
> +        {
> +            p = NULL;
> +#if (TARGET_ABI_BITS == 32)
> +            if (arg6) {
> +                p = lock_user_string(arg6);
> +                if (!p) {
> +                    goto efault;
> +                }
> +            }
> +            ret = get_errno(fanotify_mark(arg1, arg2,
> +                                target_offset64(arg3, arg4), arg5 , p));
> +            if (arg6) {
> +                unlock_user(p, arg6, 0);
> +            }

The logic in the two halves of this #if is basically the same;
I think it would be clearer to write

    uint64_t mask;
    int dirfd;
    abi_long pathname_arg;

#if TARGET_ABI_BITS == 32
    mask = target_offset64(arg3, arg4);
    dirfd = arg5;
    pathname_arg = arg6;
#else
    mask = arg3;
    dirfd = arg4;
    pathname_arg = arg5;
#endif

and then share the code that actually operates on them.

> +#else
> +            if (arg5) {
> +                p = lock_user_string(arg5);
> +                if (!p) {
> +                    goto efault;
> +                }
> +            }
> +            ret = get_errno(fanotify_mark(arg1, arg2, arg3, arg4 , p));
> +            if (arg5) {
> +                unlock_user(p, arg5, 0);
> +            }

You don't need the conditional, because unlock_user(NULL, ...) is a
no-op.

> +#endif
> +        }
> +        break;
> +#endif
> +
>  #if defined(TARGET_NR_mq_open) && defined(__NR_mq_open)
>      case TARGET_NR_mq_open:
>          {
> --
> 2.7.4

thanks
-- PMM

[Qemu-devel] [PATCH v2 2/7] linux-user: Fix inotify_init1 support

[Lena Djokic]

This commit adds necessary conversion of argument passed to inotify_init1.
inotify_init1 flags can be IN_NONBLOCK and IN_CLOEXEC which rely on O_NONBLOCK
and O_CLOEXEC and those can have different values on different platforms.

Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
---
 linux-user/syscall.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f5d9a26..41873ca 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11625,7 +11625,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #ifdef CONFIG_INOTIFY1
 #if defined(TARGET_NR_inotify_init1) && defined(__NR_inotify_init1)
     case TARGET_NR_inotify_init1:
-        ret = get_errno(sys_inotify_init1(arg1));
+        ret = get_errno(sys_inotify_init1(target_to_host_bitmask(arg1,
+                                          fcntl_flags_tbl)));
         break;
 #endif
 #endif
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/7] linux-user: Fix inotify_init1 support

[Peter Maydell]

On 24 November 2016 at 16:08, Lena Djokic <Lena.Djokic@rt-rk.com> wrote:
> This commit adds necessary conversion of argument passed to inotify_init1.
> inotify_init1 flags can be IN_NONBLOCK and IN_CLOEXEC which rely on O_NONBLOCK
> and O_CLOEXEC and those can have different values on different platforms.
>
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f5d9a26..41873ca 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11625,7 +11625,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  #ifdef CONFIG_INOTIFY1
>  #if defined(TARGET_NR_inotify_init1) && defined(__NR_inotify_init1)
>      case TARGET_NR_inotify_init1:
> -        ret = get_errno(sys_inotify_init1(arg1));
> +        ret = get_errno(sys_inotify_init1(target_to_host_bitmask(arg1,
> +                                          fcntl_flags_tbl)));
>          break;
>  #endif
>  #endif

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/7] linux-user: Fix inotify_init1 support

[Riku Voipio]

On Thu, Nov 24, 2016 at 05:08:53PM +0100, Lena Djokic wrote:
> This commit adds necessary conversion of argument passed to inotify_init1.
> inotify_init1 flags can be IN_NONBLOCK and IN_CLOEXEC which rely on O_NONBLOCK
> and O_CLOEXEC and those can have different values on different platforms.

Thanks, applied to linux-user
 
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f5d9a26..41873ca 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11625,7 +11625,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  #ifdef CONFIG_INOTIFY1
>  #if defined(TARGET_NR_inotify_init1) && defined(__NR_inotify_init1)
>      case TARGET_NR_inotify_init1:
> -        ret = get_errno(sys_inotify_init1(arg1));
> +        ret = get_errno(sys_inotify_init1(target_to_host_bitmask(arg1,
> +                                          fcntl_flags_tbl)));
>          break;
>  #endif
>  #endif
> -- 
> 2.7.4
> 

[Qemu-devel] [PATCH v2 3/7] linux-user: Fix flock definition for mips64

[Lena Djokic]

Mips64 uses generic flock structure.
See /arch/mips/include/uapi/asm/fcntl.h#L63 for reference.

Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
---
 linux-user/syscall_defs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 0b15466..099fd0e 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2363,7 +2363,7 @@ struct target_flock {
     short l_whence;
     abi_long l_start;
     abi_long l_len;
-#if defined(TARGET_MIPS)
+#if defined(TARGET_MIPS) && (TARGET_ABI_BITS == 32)
     abi_long l_sysid;
 #endif
     int l_pid;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 3/7] linux-user: Fix flock definition for mips64

[Peter Maydell]

On 24 November 2016 at 16:08, Lena Djokic <Lena.Djokic@rt-rk.com> wrote:
> Mips64 uses generic flock structure.
> See /arch/mips/include/uapi/asm/fcntl.h#L63 for reference.
>
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall_defs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 0b15466..099fd0e 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2363,7 +2363,7 @@ struct target_flock {
>      short l_whence;
>      abi_long l_start;
>      abi_long l_len;
> -#if defined(TARGET_MIPS)
> +#if defined(TARGET_MIPS) && (TARGET_ABI_BITS == 32)
>      abi_long l_sysid;
>  #endif
>      int l_pid;

Does the #if around the 'pad' field of this struct need adjusting too ?

thanks
-- PMM

[Qemu-devel] [PATCH v2 4/7] linux-user: Fix fcnt

[Lena Djokic]

F_GETSIG and F_SETSIG were implemented with default behaviour which
simply passes given arguments to fcntl syscall, but since those
arguments are signals used for communication between taget and
host we need conversion which is done by using host_to_target_signal
and taget_to_host_signal functions.

Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
---
 linux-user/syscall.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 41873ca..1b59a71 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6544,14 +6544,18 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg)
 
     case TARGET_F_SETOWN:
     case TARGET_F_GETOWN:
-    case TARGET_F_SETSIG:
-    case TARGET_F_GETSIG:
     case TARGET_F_SETLEASE:
     case TARGET_F_GETLEASE:
     case TARGET_F_SETPIPE_SZ:
     case TARGET_F_GETPIPE_SZ:
         ret = get_errno(safe_fcntl(fd, host_cmd, arg));
         break;
+    case TARGET_F_GETSIG:
+        ret = host_to_target_signal(get_errno(fcntl(fd, host_cmd, arg)));
+        break;
+    case TARGET_F_SETSIG:
+        ret = get_errno(fcntl(fd, host_cmd, target_to_host_signal(arg)));
+        break;
 
     default:
         ret = get_errno(safe_fcntl(fd, cmd, arg));
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 4/7] linux-user: Fix fcnt

[Peter Maydell]

On 24 November 2016 at 16:08, Lena Djokic <Lena.Djokic@rt-rk.com> wrote:

Making the subject line "linux-user: fix F_GETSIG and F_SETSIG fcntls"
would be a bit more precise about what we're fixing here and I think
that will be helpful for people looking back in the git log later.

> F_GETSIG and F_SETSIG were implemented with default behaviour which
> simply passes given arguments to fcntl syscall, but since those
> arguments are signals used for communication between taget and

typo: "target"

> host we need conversion which is done by using host_to_target signal
> and taget_to_host_signal functions.

"target"

>
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 41873ca..1b59a71 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6544,14 +6544,18 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg)
>
>      case TARGET_F_SETOWN:
>      case TARGET_F_GETOWN:
> -    case TARGET_F_SETSIG:
> -    case TARGET_F_GETSIG:
>      case TARGET_F_SETLEASE:
>      case TARGET_F_GETLEASE:
>      case TARGET_F_SETPIPE_SZ:
>      case TARGET_F_GETPIPE_SZ:
>          ret = get_errno(safe_fcntl(fd, host_cmd, arg));
>          break;
> +    case TARGET_F_GETSIG:
> +        ret = host_to_target_signal(get_errno(fcntl(fd, host_cmd, arg)));
> +        break;
> +    case TARGET_F_SETSIG:
> +        ret = get_errno(fcntl(fd, host_cmd, target_to_host_signal(arg)));
> +        break;

This is basically right, but I suggest some minor changes:
 * put these special cases above the long list of 'just call fcntl'
   cases, not below it
 * these should both call safe_fcntl(), not fcntl()
 * don't call host_to_target_signal() unless we know the call
   succeeded (compare the code used for TARGET_F_GETLK, TARGET_F_GETFL, etc)

>
>      default:
>          ret = get_errno(safe_fcntl(fd, cmd, arg));

thanks
-- PMM

[Qemu-devel] [PATCH v2 5/7] linux-user: Fix readahead

[Lena Djokic]

Calculation of 64-bit offset was not correct for all cases.

Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
---
 linux-user/syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1b59a71..61c4126 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11296,7 +11296,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             arg3 = arg4;
             arg4 = arg5;
         }
-        ret = get_errno(readahead(arg1, ((off64_t)arg3 << 32) | arg2, arg4));
+        ret = get_errno(readahead(arg1, target_offset64(arg2, arg3) , arg4));
 #else
         ret = get_errno(readahead(arg1, arg2, arg3));
 #endif
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 5/7] linux-user: Fix readahead

[Peter Maydell]

On 24 November 2016 at 16:08, Lena Djokic <Lena.Djokic@rt-rk.com> wrote:
> Calculation of 64-bit offset was not correct for all cases.
>
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1b59a71..61c4126 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11296,7 +11296,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              arg3 = arg4;
>              arg4 = arg5;
>          }
> -        ret = get_errno(readahead(arg1, ((off64_t)arg3 << 32) | arg2, arg4));
> +        ret = get_errno(readahead(arg1, target_offset64(arg2, arg3) , arg4));
>  #else
>          ret = get_errno(readahead(arg1, arg2, arg3));
>  #endif

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 5/7] linux-user: Fix readahead

[Riku Voipio]

On Thu, Nov 24, 2016 at 05:08:56PM +0100, Lena Djokic wrote:
> Calculation of 64-bit offset was not correct for all cases.

Thanks, applied to linux-user
 
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1b59a71..61c4126 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11296,7 +11296,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              arg3 = arg4;
>              arg4 = arg5;
>          }
> -        ret = get_errno(readahead(arg1, ((off64_t)arg3 << 32) | arg2, arg4));
> +        ret = get_errno(readahead(arg1, target_offset64(arg2, arg3) , arg4));
>  #else
>          ret = get_errno(readahead(arg1, arg2, arg3));
>  #endif
> -- 
> 2.7.4
> 

[Qemu-devel] [PATCH v2 6/7] linux-user: Fix syslog

[Lena Djokic]

Third argument represents lenght not second.
If second argument is NULL it should be passed without
using lock_user function which would, in that case, return
EFAULT, and system call supports passing NULL as second argument.

Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
---
 linux-user/syscall.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 61c4126..3faf4f0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9426,7 +9426,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #if defined(TARGET_NR_syslog)
     case TARGET_NR_syslog:
         {
-            int len = arg2;
+            int len = arg3;
 
             switch (arg1) {
             case TARGET_SYSLOG_ACTION_CLOSE:         /* Close log */
@@ -9450,13 +9450,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                         goto fail;
                     }
                     ret = 0;
-                    if (len == 0) {
-                        break;
-                    }
-                    p = lock_user(VERIFY_WRITE, arg2, arg3, 0);
-                    if (!p) {
-                        ret = -TARGET_EFAULT;
-                        goto fail;
+                    p = NULL;
+                    if (arg2) {
+                        p = lock_user(VERIFY_WRITE, arg2, arg3, 0);
+                        if (!p) {
+                            ret = -TARGET_EFAULT;
+                            goto fail;
+                        }
                     }
                     ret = get_errno(sys_syslog((int)arg1, p, (int)arg3));
                     unlock_user(p, arg2, arg3);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 6/7] linux-user: Fix syslog

[Peter Maydell]

On 24 November 2016 at 16:08, Lena Djokic <Lena.Djokic@rt-rk.com> wrote:
> Third argument represents lenght not second.

typo: "length"

> If second argument is NULL it should be passed without
> using lock_user function which would, in that case, return
> EFAULT, and system call supports passing NULL as second argument.

Looking at the kernel code, it doesn't support NULL as the
second argument for the three actions here (READ, READ_CLEAR,
READ_ALL) -- they all fail EINVAL. So what we're doing here
is just returning a better errno. I think we can do
this more simply by just changing the current
                    if (len < 0) {
                        goto fail;
                    }

to "if (!arg2 || len < 0) {" (which is what the kernel code does).

(I think it would also be reasonable to consistently use "len"
and never "arg3" (the existing code has an odd mix of both);
if you want to do that cleanup you could add an extra patch for
it, but you don't have to.)

> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 61c4126..3faf4f0 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9426,7 +9426,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  #if defined(TARGET_NR_syslog)
>      case TARGET_NR_syslog:
>          {
> -            int len = arg2;
> +            int len = arg3;
>
>              switch (arg1) {
>              case TARGET_SYSLOG_ACTION_CLOSE:         /* Close log */
> @@ -9450,13 +9450,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                          goto fail;
>                      }
>                      ret = 0;
> -                    if (len == 0) {
> -                        break;
> -                    }
> -                    p = lock_user(VERIFY_WRITE, arg2, arg3, 0);
> -                    if (!p) {
> -                        ret = -TARGET_EFAULT;
> -                        goto fail;
> +                    p = NULL;
> +                    if (arg2) {
> +                        p = lock_user(VERIFY_WRITE, arg2, arg3, 0);
> +                        if (!p) {
> +                            ret = -TARGET_EFAULT;
> +                            goto fail;
> +                        }
>                      }
>                      ret = get_errno(sys_syslog((int)arg1, p, (int)arg3));
>                      unlock_user(p, arg2, arg3);
> --
> 2.7.4

thanks
-- PMM

[Qemu-devel] [PATCH v2 7/7] linux-user: Fix mq_open

[Lena Djokic]

If fourth argument is NULL it should be passed without
using lock_user function which would, in that case, return
EFAULT, and system call supports passing NULL as fourth argument.

Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
---
 linux-user/syscall.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3faf4f0..dad03e9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11694,17 +11694,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
     case TARGET_NR_mq_open:
         {
             struct mq_attr posix_mq_attr;
+            struct mq_attr *pposix_mq_attr;
             int host_flags;
 
             host_flags = target_to_host_bitmask(arg2, fcntl_flags_tbl);
-            if (copy_from_user_mq_attr(&posix_mq_attr, arg4) != 0) {
-                goto efault;
+            pposix_mq_attr = NULL;
+            if (arg4) {
+                if (copy_from_user_mq_attr(&posix_mq_attr, arg4) != 0) {
+                    goto efault;
+                }
+                pposix_mq_attr = &posix_mq_attr;
             }
             p = lock_user_string(arg1 - 1);
             if (!p) {
                 goto efault;
             }
-            ret = get_errno(mq_open(p, host_flags, arg3, &posix_mq_attr));
+            ret = get_errno(mq_open(p, host_flags, arg3, pposix_mq_attr));
             unlock_user (p, arg1, 0);
         }
         break;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 7/7] linux-user: Fix mq_open

[Peter Maydell]

On 24 November 2016 at 16:08, Lena Djokic <Lena.Djokic@rt-rk.com> wrote:
> If fourth argument is NULL it should be passed without
> using lock_user function which would, in that case, return
> EFAULT, and system call supports passing NULL as fourth argument.
>
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 3faf4f0..dad03e9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11694,17 +11694,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_mq_open:
>          {
>              struct mq_attr posix_mq_attr;
> +            struct mq_attr *pposix_mq_attr;
>              int host_flags;
>
>              host_flags = target_to_host_bitmask(arg2, fcntl_flags_tbl);
> -            if (copy_from_user_mq_attr(&posix_mq_attr, arg4) != 0) {
> -                goto efault;
> +            pposix_mq_attr = NULL;
> +            if (arg4) {
> +                if (copy_from_user_mq_attr(&posix_mq_attr, arg4) != 0) {
> +                    goto efault;
> +                }
> +                pposix_mq_attr = &posix_mq_attr;
>              }
>              p = lock_user_string(arg1 - 1);
>              if (!p) {
>                  goto efault;
>              }
> -            ret = get_errno(mq_open(p, host_flags, arg3, &posix_mq_attr));
> +            ret = get_errno(mq_open(p, host_flags, arg3, pposix_mq_attr));
>              unlock_user (p, arg1, 0);
>          }
>          break;
> --
> 2.7.4

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 7/7] linux-user: Fix mq_open

[Riku Voipio]

On Thu, Nov 24, 2016 at 05:08:58PM +0100, Lena Djokic wrote:
> If fourth argument is NULL it should be passed without
> using lock_user function which would, in that case, return
> EFAULT, and system call supports passing NULL as fourth argument.

Thanks, applied to linux-user
 
> Signed-off-by: Lena Djokic <Lena.Djokic@rt-rk.com>
> ---
>  linux-user/syscall.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 3faf4f0..dad03e9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11694,17 +11694,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_mq_open:
>          {
>              struct mq_attr posix_mq_attr;
> +            struct mq_attr *pposix_mq_attr;
>              int host_flags;
>  
>              host_flags = target_to_host_bitmask(arg2, fcntl_flags_tbl);
> -            if (copy_from_user_mq_attr(&posix_mq_attr, arg4) != 0) {
> -                goto efault;
> +            pposix_mq_attr = NULL;
> +            if (arg4) {
> +                if (copy_from_user_mq_attr(&posix_mq_attr, arg4) != 0) {
> +                    goto efault;
> +                }
> +                pposix_mq_attr = &posix_mq_attr;
>              }
>              p = lock_user_string(arg1 - 1);
>              if (!p) {
>                  goto efault;
>              }
> -            ret = get_errno(mq_open(p, host_flags, arg3, &posix_mq_attr));
> +            ret = get_errno(mq_open(p, host_flags, arg3, pposix_mq_attr));
>              unlock_user (p, arg1, 0);
>          }
>          break;
> -- 
> 2.7.4
>