diff mbox

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

Message ID 9B0F7F7B0E031F4A99FC8A50E22E3AA360FC2658@EU-MBX-01.mgc.mentorg.com
State New
Headers show

Commit Message

Sair, Umair Oct. 5, 2015, 6:03 p.m. UTC 
> 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:


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

Comments

Paolo Bonzini Oct. 5, 2015, 7:22 p.m. UTC | #1 
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
Paolo Bonzini Oct. 5, 2015, 7:27 p.m. UTC | #2 
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
diff mbox

Patch

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);