Patchwork Fix ping issue for linux-user guest

login
register
mail settings
Submitter Jing Huang
Date July 11, 2012, 1:56 p.m.
Message ID <CAK8mDgperYdSHNQewtzYRQosn1q6=yy7MSZs9HLC=vUwqCe6SQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/170472/
State New
Headers show

Comments

Jing Huang - July 11, 2012, 1:56 p.m.
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.


Signed-off-by: Jing Huang <jing.huang.pku@gmail.com>
---
 syscall.c        |    24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

+            }
             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
+        case ICMP_FILTER:
+            /*struct icmp_filter takes an u32 value*/
+            optname = ICMP_FILTER;
+            if (optlen < sizeof(uint32_t))
+                return -TARGET_EINVAL;
+
+            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.  */
Peter Maydell - July 11, 2012, 2:44 p.m.
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
dunrong huang - July 11, 2012, 3:01 p.m.
2012/7/11 Jing Huang <jing.huang.pku@gmail.com>:
> 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.
>
Your patch is weird, maybe this is caused by your mail client.
You'd better use the tool "git send-email" to send patch.

Patch

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);
+            if(cmsg->cmsg_type == SO_TIMESTAMP) {
+                /*copy msg_name to target_msgh*/
+                target_msgh->msg_namelen = msgh->msg_namelen;
+                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);