Re: [Qemu-devel] [PATCH] Fix ping issue for linux-user guest

[Peter Maydell <peter.maydell@linaro.org>]

Thanks for this patch. Review comments below...

(cc'ing the linux-user maintainer)

On 11 July 2012 14:56, Jing Huang <jing.huang.pku@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.pku@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