Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Peter Maydell]

(looping in qemu-devel and Paolo)

On 2 October 2015 at 17:54, Sair, Umair <Umair_Sair@mentor.com> wrote:
> I am working with qemu-2.4.0. I built it using mingw on Windows. I am having
> a problem while connecting it to gdb. I figured out that qemu gdb server is
> running on IPv6 whereas gdb tries to connect on IPv4. This problem is
> already reported here: https://bugs.launchpad.net/qemu/+bug/562107.
>
> gdd server runs on IPv6 even if I provide ipv4 tcp option (-S -gdb
> tcp::10000,ipv4). In other words, providing ipv4 or ipv6 option have no
> effect on both windows and linux. On linux it always runs on IPv4 and on
> Windows, it always runs on IPv6. Can you please help me out in resolving
> this problem?
>
> I debugged qemu a bit and it seems to me that the problem is in
> ‘inet_addr_to_opts()’ function of ‘qemu-sockets.c’. Following is the excerpt
> from this function:
>
> static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)
> {
>
>     bool ipv4 = addr->ipv4 || !addr->has_ipv4;
>     bool ipv6 = addr->ipv6 || !addr->has_ipv6;
>
>     if (!ipv4 || !ipv6) {
>         qemu_opt_set_bool(opts, "ipv4", ipv4);
>         qemu_opt_set_bool(opts, "ipv6", ipv6);
>     }
>
> The ipv4 and ipv6 variables of this function are always true (because we set
> addr->ipv4 = addr->has_ipv4 and addr->ipv6 = addr->has_ipv6), hence ipv4 and
> ipv6 option is never set in the if condition. The consequence is that the
> socket is created with PF_UNSPEC and then for windows IPv6 and for linux
> IPv4 socket is created (see inet_listen_opts() in qemu-sockets.c). Is there
> a particular reason for the logic of inet_addr_to_opts()?

I'm not sure, but something definitely seems weird here; either
inet_parse or inet_addr_to_opts is wrong, I think.

Paolo, you wrote this code I think; any idea?

My guess is that inet_parse should be setting addr->has_ipv6
to true (leaving addr->ipv6 false) if it decides it should be
using IPv4 (and vice-versa for IPv6).

inet_addr_to_opts I think is correct where it does:
    bool ipv4 = addr->ipv4 || !addr->has_ipv4;
because this is saying "try ipv4 if specifically asked, or if
the caller didn't specify either way".

I don't know why we have the "if (!ipv4 || !ipv6)" condition on
the setting of the opts, though.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Sair, Umair]

If I am understanding correct, you are saying that we should set addr->has_ipv6 and addr->has_ipv4 to true in any case, then in my opinion we should simply ignore the value of addr->has_ipv* while evaluating the value of ipv4 and ipv6 variables in inet_addr_to_opts (bool ipv4 = addr->ipv4; bool ipv6 = addr->ipv6). And the "if (!ipv4 || !ipv6)" condition seems really strange to me.

Regards,
Umair Sair

-----Original Message-----
From: Peter Maydell [mailto:peter.maydell@linaro.org] 
Sent: Friday, October 02, 2015 10:57 PM
To: Sair, Umair
Cc: qemu-discuss@nongnu.org; Paolo Bonzini; QEMU Developers
Subject: Re: [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

(looping in qemu-devel and Paolo)

On 2 October 2015 at 17:54, Sair, Umair <Umair_Sair@mentor.com> wrote:
> I am working with qemu-2.4.0. I built it using mingw on Windows. I am 
> having a problem while connecting it to gdb. I figured out that qemu 
> gdb server is running on IPv6 whereas gdb tries to connect on IPv4. 
> This problem is already reported here: https://bugs.launchpad.net/qemu/+bug/562107.
>
> gdd server runs on IPv6 even if I provide ipv4 tcp option (-S -gdb 
> tcp::10000,ipv4). In other words, providing ipv4 or ipv6 option have 
> no effect on both windows and linux. On linux it always runs on IPv4 
> and on Windows, it always runs on IPv6. Can you please help me out in 
> resolving this problem?
>
> I debugged qemu a bit and it seems to me that the problem is in 
> ‘inet_addr_to_opts()’ function of ‘qemu-sockets.c’. Following is the 
> excerpt from this function:
>
> static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress 
> *addr) {
>
>     bool ipv4 = addr->ipv4 || !addr->has_ipv4;
>     bool ipv6 = addr->ipv6 || !addr->has_ipv6;
>
>     if (!ipv4 || !ipv6) {
>         qemu_opt_set_bool(opts, "ipv4", ipv4);
>         qemu_opt_set_bool(opts, "ipv6", ipv6);
>     }
>
> The ipv4 and ipv6 variables of this function are always true (because 
> we set
> addr->ipv4 = addr->has_ipv4 and addr->ipv6 = addr->has_ipv6), hence 
> addr->ipv4 and
> ipv6 option is never set in the if condition. The consequence is that 
> the socket is created with PF_UNSPEC and then for windows IPv6 and for 
> linux
> IPv4 socket is created (see inet_listen_opts() in qemu-sockets.c). Is 
> there a particular reason for the logic of inet_addr_to_opts()?

I'm not sure, but something definitely seems weird here; either inet_parse or inet_addr_to_opts is wrong, I think.

Paolo, you wrote this code I think; any idea?

My guess is that inet_parse should be setting addr->has_ipv6 to true (leaving addr->ipv6 false) if it decides it should be using IPv4 (and vice-versa for IPv6).

inet_addr_to_opts I think is correct where it does:
    bool ipv4 = addr->ipv4 || !addr->has_ipv4; because this is saying "try ipv4 if specifically asked, or if the caller didn't specify either way".

I don't know why we have the "if (!ipv4 || !ipv6)" condition on the setting of the opts, though.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Peter Maydell]

On 2 October 2015 at 19:20, Sair, Umair <Umair_Sair@mentor.com> wrote:
> If I am understanding correct, you are saying that we should set addr->has_ipv6 and addr->has_ipv4 to true in any case, then in my opinion we should simply ignore the value of addr->has_ipv* while evaluating the value of ipv4 and ipv6 variables in inet_addr_to_opts (bool ipv4 = addr->ipv4; bool ipv6 = addr->ipv6). And the "if (!ipv4 || !ipv6)" condition seems really strange to me.

No, the has_ipv* flags are important. The major use of
this structure is as the internal representation of a
JSON-like data structure. This always has a "has_foo"
bool field for any optional "foo" data field. So the
three states foo can have are:
 * has_foo = false              => foo not specified
 * has_foo = true, foo = false  => foo specified as false
 * has_foo = true, foo = true   => foo specified as true

(Eventually somebody will clean this code up so that
rather than "create an InetSocketAddress*, then convert
it to QemuOpts, then do the actual operation based on
the QemuOpts" we will just do "create InetSocketAddress*,
then do the operation based on the InetSocketAddress*".
That's why we have this indirection in the code, which
we want to preserve. Unfortunately first we have to convert
a lot of legacy callers that directly call the "do op
based on QemuOpts" functions so they call "do op
based on InetSocketAddress*".)

I agree about the (!ipv4 || !ipv6) condition though.
The three states I listed above ought to correspond
to "qemu_opt not set", "qemu_opt set to false" and
"qemu_opt set to true",

-- PMM

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Liviu Ionescu]

> On 02 Oct 2015, at 21:20, Sair, Umair <Umair_Sair@mentor.com> wrote:
> 
>> On linux it always runs on IPv4 
>> and on Windows, it always runs on IPv6. Can you please help me out in 
>> resolving this problem?

I remember I had the same problem when building GNU ARM Eclipse QEMU.

You can take a look at:

https://github.com/gnuarmeclipse/qemu/blob/gnuarmeclipse-dev/util/qemu-sockets.c


Regards,

Liviu

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Paolo Bonzini]

On 03/10/2015 00:36, Peter Maydell wrote:
> 
> I agree about the (!ipv4 || !ipv6) condition though.
> The three states I listed above ought to correspond
> to "qemu_opt not set", "qemu_opt set to false" and
> "qemu_opt set to true",

The problem is that the underlying QemuOpts-based code treats "qemu_opt not set" and "qemu_opt set to false" the same way:

  ipv4   ipv6
  Y      Y       PF_INET6
  Y      N       PF_INET
  N      Y       PF_INET6
  N      N       PF_UNSPEC

We want:

 ipv4 ipv6
 Y    Y     PF_INET6
 Y    N     PF_INET
 Y    -     PF_INET         (ipv6 = N)
 N    Y     PF_INET6
 N    N     PF_UNSPEC
 N    -     PF_INET6        (ipv6 = Y)
 -    Y     PF_INET6        (ipv4 = N)
 -    N     PF_INET         (ipv4 = Y)
 -    -     PF_UNSPEC

I think this patch gets the desired semantics:

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 2add83a..fdcf3fa 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -586,12 +586,15 @@ fail:
 
 static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)
 {
-    bool ipv4 = addr->ipv4 || !addr->has_ipv4;
-    bool ipv6 = addr->ipv6 || !addr->has_ipv6;
+    bool ipv4 = addr->has_ipv4 && addr->ipv4;
+    bool ipv6 = addr->has_ipv6 && addr->ipv6;
 
-    if (!ipv4 || !ipv6) {
+    if (ipv4 || ipv6) {
         qemu_opt_set_bool(opts, "ipv4", ipv4, &error_abort);
         qemu_opt_set_bool(opts, "ipv6", ipv6, &error_abort);
+    } else if (addr->has_ipv4 || addr->has_ipv6) {
+        qemu_opt_set_bool(opts, "ipv4", !addr->has_ipv4, &error_abort);
+        qemu_opt_set_bool(opts, "ipv6", !addr->has_ipv6, &error_abort);
     }
     if (addr->has_to) {
         qemu_opt_set_number(opts, "to", addr->to, &error_abort);


The first if handles the "default to N" case, the second handles
"default to Y", the (absent) else case handles "default to
PF_UNSPEC".

Paolo

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Sair, Umair]

> The first if handles the "default to N" case, the second handles "default to Y", the (absent) else case handles "default to PF_UNSPEC".

Can you please elaborate it. Also I am not understanding the reason for inverting the values of addr->has_ipv* in second if condition.

I believe that the fix for the issue under discussion will be committed to qemu repo very soon, so I'll like to add one more thing which requires to be fixed along with it. In 'tcp_chr_accept' function of qemu-char.c, the data type of saddr should be sockaddr_in6 so that it works with both IPv6 and IPv4 on Windows (works for linux without it because of accept4 and works with this solution as well!).

Regards,
Umair Sair

-----Original Message-----
From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
Sent: Sunday, October 04, 2015 3:48 PM
To: Peter Maydell; Sair, Umair
Cc: QEMU Developers; qemu-discuss@nongnu.org
Subject: Re: [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

On 03/10/2015 00:36, Peter Maydell wrote:
> 
> I agree about the (!ipv4 || !ipv6) condition though.
> The three states I listed above ought to correspond to "qemu_opt not 
> set", "qemu_opt set to false" and "qemu_opt set to true",

The problem is that the underlying QemuOpts-based code treats "qemu_opt not set" and "qemu_opt set to false" the same way:

  ipv4   ipv6
  Y      Y       PF_INET6
  Y      N       PF_INET
  N      Y       PF_INET6
  N      N       PF_UNSPEC

We want:

 ipv4 ipv6
 Y    Y     PF_INET6
 Y    N     PF_INET
 Y    -     PF_INET         (ipv6 = N)
 N    Y     PF_INET6
 N    N     PF_UNSPEC
 N    -     PF_INET6        (ipv6 = Y)
 -    Y     PF_INET6        (ipv4 = N)
 -    N     PF_INET         (ipv4 = Y)
 -    -     PF_UNSPEC

I think this patch gets the desired semantics:

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 2add83a..fdcf3fa 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -586,12 +586,15 @@ fail:
 
 static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)  {
-    bool ipv4 = addr->ipv4 || !addr->has_ipv4;
-    bool ipv6 = addr->ipv6 || !addr->has_ipv6;
+    bool ipv4 = addr->has_ipv4 && addr->ipv4;
+    bool ipv6 = addr->has_ipv6 && addr->ipv6;
 
-    if (!ipv4 || !ipv6) {
+    if (ipv4 || ipv6) {
         qemu_opt_set_bool(opts, "ipv4", ipv4, &error_abort);
         qemu_opt_set_bool(opts, "ipv6", ipv6, &error_abort);
+    } else if (addr->has_ipv4 || addr->has_ipv6) {
+        qemu_opt_set_bool(opts, "ipv4", !addr->has_ipv4, &error_abort);
+        qemu_opt_set_bool(opts, "ipv6", !addr->has_ipv6, &error_abort);
     }
     if (addr->has_to) {
         qemu_opt_set_number(opts, "to", addr->to, &error_abort);


The first if handles the "default to N" case, the second handles "default to Y", the (absent) else case handles "default to PF_UNSPEC".

Paolo

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Paolo Bonzini]

On 05/10/2015 20:03, Sair, Umair wrote:
>> The first if handles the "default to N" case, the second handles "default to Y", the (absent) else case handles "default to PF_UNSPEC".
> 
> Can you please elaborate it. Also I am not understanding the reason for inverting the values of addr->has_ipv* in second if condition.

If a value is absent, you can either default it to Y (then the value is
!addr->has_xyz || addr->xyz) or default it to N (then the value is
addr->has_xyz && addr->xyz).

The desired logic is:

- with one absent value and one "on", the absent value should be "off"

- with one absent value and one "off", the absent value should be "on"

- with two absent values, the absent values can be both left absent

The patch works like this:

>>  static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)  {
>> -    bool ipv4 = addr->ipv4 || !addr->has_ipv4;
>> -    bool ipv6 = addr->ipv6 || !addr->has_ipv6;
>> +    bool ipv4 = addr->has_ipv4 && addr->ipv4;
>> +    bool ipv6 = addr->has_ipv6 && addr->ipv6;
>>  
>> -    if (!ipv4 || !ipv6) {
>> +    if (ipv4 || ipv6) {
>>          qemu_opt_set_bool(opts, "ipv4", ipv4, &error_abort);
>>          qemu_opt_set_bool(opts, "ipv6", ipv6, &error_abort);

This handles the case where the absent value should be "off": at least
one value is present _and_ true.

>> +    } else if (addr->has_ipv4 || addr->has_ipv6) {
>> +        qemu_opt_set_bool(opts, "ipv4", !addr->has_ipv4, &error_abort);
>> +        qemu_opt_set_bool(opts, "ipv6", !addr->has_ipv6, &error_abort);

This handles the case where the absent value should be "on".  We know
that whichever the present value is, it is false; therefore
!addr->has_xyz || addr->xyz simplifies to just !addr->has_xyz.

> I believe that the fix for the issue under discussion will be
> committed to qemu repo very soon,

Did you test the patch, and did it work for you?  If so, it is customary
to reply with a line like "Tested by: Sair, Umair <Umair_Sair@mentor.com>".

> so I'll like to add one more thing
> which requires to be fixed along with it. In 'tcp_chr_accept'
> function of qemu-char.c, the data type of saddr should be
> sockaddr_in6 so that it works with both IPv6 and IPv4 on Windows
> (works for linux without it because of accept4 and works with this
> solution as well!).

Can you send a patch for it?

Thanks!

Paolo

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Paolo Bonzini]

On 05/10/2015 20:03, Sair, Umair wrote:
>> The first if handles the "default to N" case, the second handles "default to Y", the (absent) else case handles "default to PF_UNSPEC".
> 
> Can you please elaborate it. Also I am not understanding the reason for inverting the values of addr->has_ipv* in second if condition.

If a value is absent, you can either default it to Y (then the value is
!addr->has_xyz || addr->xyz) or default it to N (then the value is
addr->has_xyz && addr->xyz).

The desired logic is:

- with one absent value and one "on", the absent value should be "off"

- with one absent value and one "off", the absent value should be "on"

- with two absent values, the absent values can be both left absent

The patch works like this:

>>  static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)  {
>> -    bool ipv4 = addr->ipv4 || !addr->has_ipv4;
>> -    bool ipv6 = addr->ipv6 || !addr->has_ipv6;
>> +    bool ipv4 = addr->has_ipv4 && addr->ipv4;
>> +    bool ipv6 = addr->has_ipv6 && addr->ipv6;
>>  
>> -    if (!ipv4 || !ipv6) {
>> +    if (ipv4 || ipv6) {
>>          qemu_opt_set_bool(opts, "ipv4", ipv4, &error_abort);
>>          qemu_opt_set_bool(opts, "ipv6", ipv6, &error_abort);

This handles the case where the absent value should be "off": at least
one value is present _and_ true.

>> +    } else if (addr->has_ipv4 || addr->has_ipv6) {
>> +        qemu_opt_set_bool(opts, "ipv4", !addr->has_ipv4, &error_abort);
>> +        qemu_opt_set_bool(opts, "ipv6", !addr->has_ipv6, &error_abort);

This handles the case where the absent value should be "on".  We know
that whichever the present value is, it is false; therefore
!addr->has_xyz || addr->xyz simplifies to just !addr->has_xyz.

> I believe that the fix for the issue under discussion will be
> committed to qemu repo very soon,

Did you test the patch, and did it work for you?  If so, it is customary
to reply with a line like "Tested by: Sair, Umair <Umair_Sair@mentor.com>".

> so I'll like to add one more thing
> which requires to be fixed along with it. In 'tcp_chr_accept'
> function of qemu-char.c, the data type of saddr should be
> sockaddr_in6 so that it works with both IPv6 and IPv4 on Windows
> (works for linux without it because of accept4 and works with this
> solution as well!).

Can you send a patch for it?

Thanks!

Paolo

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Sair, Umair]

Paolo, thanks for the explanation :)

> Did you test the patch, and did it work for you?  If so, it is customary to reply with a line like "Tested by: Sair, Umair <Umair_Sair@mentor.com>".
Yes, it worked for me. Should I reply to the email containing the patch with the above line?

>> so I'll like to add one more thing
>> which requires to be fixed along with it. In 'tcp_chr_accept'
>> function of qemu-char.c, the data type of saddr should be
>> sockaddr_in6 so that it works with both IPv6 and IPv4 on Windows 
>> (works for linux without it because of accept4 and works with this 
>> solution as well!).
>
> Can you send a patch for it?

diff --git a/qemu-char.c b/qemu-char.c
index 653ea10..14966e9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2985,7 +2985,7 @@ static gboolean tcp_chr_accept(GIOChannel *channel, GIOCon
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
-    struct sockaddr_in saddr;
+    struct sockaddr_in6 saddr;
 #ifndef _WIN32
     struct sockaddr_un uaddr;
 #endif

Regards,
Umair Sair

Re: [Qemu-devel] [Qemu-discuss] TCP options ipv4 and ipv6 have no effect

[Paolo Bonzini]

On 12/10/2015 15:06, Sair, Umair wrote:
> Paolo, thanks for the explanation :)
> 
>> > Did you test the patch, and did it work for you?  If so, it is customary to reply with a line like "Tested by: Sair, Umair <Umair_Sair@mentor.com>".
> Yes, it worked for me. Should I reply to the email containing the patch with the above line?

No need to, thanks!

Paolo