Thanks for this patch. Review comments below...
(cc'ing the linux-user maintainer)
On 11 July 2012 14:56, Jing Huang <jing.huang....@gmail.com> wrote:
> This patch fix ping issues for linux-user guest.
>
> * The do_setsockopts function in linux-user does not support SOL_RAW
> socket which is used in ping net tool.
>
> * The recvmsg in main_loop of ping could not fetch
> sockaddr_in struct. That is because do_sendrecvmsg in linux-user does
> not pass the msg->msg_name to the target.
>
> We fix the above issues.
These are two separate issues in two separate areas, so they
should be in separate patches.
> Signed-off-by: Jing Huang <jing.huang....@gmail.com>
> ---
> syscall.c | 24 +++++++++++++++++++++++-
> 1 files changed, 23 insertions(+), 1 deletions(-)
>
> diff -git a/linux-user/syscall.c b/linux-user/syscall.c
> index 5346554..3343345 e43f56
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1349,7 +1349,13 @@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);
This patch won't apply, because your mail client or mail server has
wrapped long lines.
Also, qemu coding style doesn't allow >80 column lines. Please
run your patch through scripts/checkpatch.pl.
> + if(cmsg->cmsg_type == SO_TIMESTAMP) {
> + /*copy msg_name to target_msgh*/
> + target_msgh->msg_namelen = msgh->msg_namelen;
This is odd. We're in the middle of a loop which is for handling a
list of cmsgs, but we're operating on the msgh. So if there's
more than one SO_TIMESTAMP cmsg in the list we'll do this twice.
It seems unlikely that this should really be restricted to just
SO_TIMESTAMP.
I think what you actually want is for do_sendrcvmsg() to do
this conversion. This would parallel the way that it already
does an incoming conversion on msg_namelen/msg_name.
Also you need to do byteswapping (see the do_sendrcvmsg() code)
not just copy the length and memcpy the data.
> + memcpy(g2h((void *)(unsigned
> long)(target_ulong)target_msgh->msg_name), msgh->msg_name,
> msgh->msg_namelen);
> + } 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;
> @@ -1442,6 +1448,23 @@
> goto unimplemented;
> }
> break;
> + case SOL_RAW:
> + switch (optname) {
> +#define ICMP_FILTER 1
This looks wrong -- shouldn't the system headers provide this?
Even if not, defining a constant in the middle of code isn't very
good style.
> + case ICMP_FILTER:
> + /*struct icmp_filter takes an u32 value*/
> + optname = ICMP_FILTER;
> + if (optlen < sizeof(uint32_t))
> + return -TARGET_EINVAL;
Coding style requires braces. Again, checkpatch.pl will tell you this.
> +
> + if (get_user_u32(val, optval_addr))
> + return -TARGET_EFAULT;
> + ret = get_errno(setsockopt(sockfd, level, optname, (char
> *)&val, sizeof(val)));
> + break;
> + default:
> + goto unimplemented;
> + }
> + break;
> case TARGET_SOL_SOCKET:
> switch (optname) {
> /* Options with 'int' argument. */
>
-- PMM
