Patchwork [V3,3/3] linux-user: make host_to_target_cmsg support SO_TIMESTAMP cmsg_type

login
register
mail settings
Submitter Jing Huang
Date July 21, 2012, 1:30 a.m.
Message ID <1342834254-2751-1-git-send-email-jing.huang.pku@gmail.com>
Download mbox | patch
Permalink /patch/172317/
State New
Headers show

Comments

Peter Maydell - July 20, 2012, 6:05 p.m.
On 21 July 2012 02:30, Jing Huang <jing.huang.pku@gmail.com> wrote:

This version of the patch is pretty close to correct -- just a few
minor things to fix...

>
> Signed-off-by: Jing Huang <jing.huang.pku@gmail.com>
> ---
>  linux-user/syscall.c |   15 +++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2228b1f..7521746 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1350,8 +1350,17 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,

Something weird happened here -- I couldn't apply this patch unless
I edited it by hand to say "+1350,19" in this diff hunk header.
You didn't edit the mail by hand after creating the diff, did you?

>          target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
>
>          if (cmsg->cmsg_level != TARGET_SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
> -            gemu_log("Unsupported ancillary data: %d/%d\n", cmsg->cmsg_level, cmsg->cmsg_type);
> -            memcpy(target_data, data, len);
> +            if ((cmsg->cmsg_type == SO_TIMESTAMP) &&
> +                                    (len == sizeof(struct timeval))) {
> +                /* copy struct timeval to target */
> +                struct timeval *tv = (struct timeval *)data;
> +                struct timeval *target_tv = (struct timeval *)target_data;

This should be "struct target_timeval *target_tv" -- the timeval
struct is (on Linux) a pair of 'long's so it is laid out differently
for different target architectures.

(I had not realised this when commenting on earlier versions of this
patch; it does mean that this patch is the right way to do this,
because copying 32 bit integers would not handle the case of guest
and host having different 'long' sizes.)

> +
> +                tv->tv_sec = tswap32(target_tv->tv_sec);
> +                tv->tv_usec = tswap32(target_tv->tv_usec);
> +            } else {
> +                gemu_log("Unsupported ancillary data: %d/%d\n",
> +                                            cmsg->cmsg_level, cmsg->cmsg_type);
> +                memcpy(target_data, data, len);
> +            }
>          } else {
>              int *fd = (int *)data;
>              int *target_fd = (int *)target_data;

The logic here isn't laid out quite right: you have
  if (not SCM rights) {
     if (timestamp) {
        handle timestamp;
     } else {
        complain;
     }
  } else {
     handle SCM rights;
  }

and what you want is:
  if (SCM rights) {
     handle SCM rights;
  } else if (timestamp) {
     handle timestamp;
  } else {
     complain;
  }

(which has the same effect but is much easier to read and understand).

Also, your check for "is this a timestamp" should include
a check that cmsg_level is TARGET_SOL_SOCKET.

-- PMM
Jing Huang - July 21, 2012, 1:30 a.m.
Signed-off-by: Jing Huang <jing.huang.pku@gmail.com>
---
 linux-user/syscall.c |   15 +++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2228b1f..7521746 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1350,8 +1350,17 @@  static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
 
         if (cmsg->cmsg_level != TARGET_SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
-            gemu_log("Unsupported ancillary data: %d/%d\n", cmsg->cmsg_level, cmsg->cmsg_type);
-            memcpy(target_data, data, len);
+            if ((cmsg->cmsg_type == SO_TIMESTAMP) &&
+                                    (len == sizeof(struct timeval))) {
+                /* copy struct timeval to target */
+                struct timeval *tv = (struct timeval *)data;
+                struct timeval *target_tv = (struct timeval *)target_data;
+
+                tv->tv_sec = tswap32(target_tv->tv_sec);
+                tv->tv_usec = tswap32(target_tv->tv_usec);
+            } else {
+                gemu_log("Unsupported ancillary data: %d/%d\n",
+                                            cmsg->cmsg_level, cmsg->cmsg_type);
+                memcpy(target_data, data, len);
+            }
         } else {
             int *fd = (int *)data;
             int *target_fd = (int *)target_data;