On 20 October 2015 at 19:50, Daniel P. Berrange <berra...@redhat.com> wrote:
> On Tue, Oct 20, 2015 at 06:01:37PM +0100, Peter Maydell wrote:
>> On 20 October 2015 at 14:39, Daniel P. Berrange <berra...@redhat.com> wrote:
>> > On Mon, Oct 19, 2015 at 04:20:09PM -0600, Eric Blake wrote:
>> >> On 10/12/2015 05:14 AM, Daniel P. Berrange wrote:
>> >> > The VNC code is currently using QemuOpts to configure the
>> >> > sockets connections / listeners it needs. Convert it to
>> >> > use SocketAddress to bring it in line with modern QAPI
>> >> > based code elsewhere in QEMU.
>>
>> >>
>> >> > + if (to) {
>> >> > + saddr->inet->has_to = true;
>> >> > + saddr->inet->to = to;
>> >> > + }
>> >> > + saddr->inet->ipv4 = saddr->inet->has_ipv4 = has_ipv4;
>> >> > + saddr->inet->ipv6 = saddr->inet->has_ipv6 = has_ipv6;
>> >>
>> >> Do we want to specify has_ipvX as true even when setting inet->ipvX to
>> >> false?
>> >
>> > This saddr instance is passed into 'socket_connect' which then
>> > converts it back into a QemuOpts object using
>> >
>> > bool ipv4 = addr->has_ipv4 && addr->ipv4;
>> >
>> > So, we don't need to set has_ipvX to true, when ipvX is false.
>>
>> That is not the *only* thing that inet_addr_to_opts does with
>> the has_* flags... this kind of thing can be the difference
>> between "force ipv4" and "use ipv4 or ipv6".
>
> The original code which used QemuOpts directly had this logic:
>
> inet_listen_opts / inet_connect_opts use this logic:
>
> ai.ai_family = PF_UNSPEC;
> ....
> if (qemu_opt_get_bool(opts, "ipv4", 0))
> ai.ai_family = PF_INET;
> if (qemu_opt_get_bool(opts, "ipv6", 0))
> ai.ai_family = PF_INET6;
>
> IOW, these methods treat ipv4 being omitted, as equivalent to ipv4=off.
>
> Thus the existing VNC -> qemu-sockets code has this logic:
>
> VNC | qemu-sockets
> QemuOpts | ai_family
> ----------------------------
> ipv4 ipv6 |
> ----------------------------
> - - | PF_UNSPEC
> - off | PF_UNSPEC
> - on | PF_INET6
> off - | PF_UNSPEC
> off off | PF_UNSPEC
> off on | PF_INET6
> on - | PF_INET
> on off | PF_INET
> on on | PF_INET6
>
>
> My patch, which also does not distinguish ipv4=off from ipv4 omitted
> has the following logic, at various stages of conversion between
> QemuOpts & QAPI SocketAddress:
>
>
> VNC | VNC | qemu-sockets | qemu-sockets
> QemuOpts | QAPI SocketAddress | QemuOpts | ai_family
> -------------------------------------------------------------------------
> ipv4 ipv6 | has_ipv4 ipv4 has_ipv6 ipv6 | ipv4 ipv6 |
> -------------------------------------------------------------------------
> - - | f f f f | - - | PF_UNSPEC
> - off | f f f f | - - | PF_UNSPEC
> - on | f f t t | off on | PF_INET6
> off - | f f f f | - - | PF_UNSPEC
> off off | f f f f | - - | PF_UNSPEC
> off on | f f t t | off on | PF_INET6
> on - | t t f f | on off | PF_INET
> on off | t t f f | on off | PF_INET
> on on | t t t t | on on | PF_INET6
>
> So, unless I've missed something, the final ai_family is set the same
> way before & after my patch, for all combinations of VNC ipv4/ipv6
> CLI options.
I think you're right that this patch doesn't change anything,
but doesn't it mean that vnc still has one of the bugs we fixed in
commit b77e7c8e99f9a: if you say 'ipv6=off' it will give you a
PF_UNSPEC rather than an PF_INET socket? (In particular on Windows
PF_UNSPEC will get you ipv6, which is exactly what the user asked
not to get...)
Since that's a pre-existing bug I guess we can fix it in a
subsequent patch.
thanks
-- PMM
