[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V3 3/3] linux-user: make host_to_target_cmsg sup
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH V3 3/3] linux-user: make host_to_target_cmsg support SO_TIMESTAMP cmsg_type |
Date: |
Fri, 20 Jul 2012 19:05:51 +0100 |
On 21 July 2012 02:30, Jing Huang <address@hidden> wrote:
This version of the patch is pretty close to correct -- just a few
minor things to fix...
>
> Signed-off-by: Jing Huang <address@hidden>
> ---
> 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