Re: QEMU Sockets Networking Backend Multicast Networking Fix

[Jason Wang <jasowang@redhat.com>]

On 2020/2/17 下午6:05, Faisal Al-Humaimidi wrote:
> Hello Jason,
>
> But, the local address is not meant to be added to the group, rather 
> we listen to it, hence we bind to the local address. The multicast 
> group is a higher layer that would be requested to join to by the 
> listening host. Here's a similar example in multicasting that 
> demonstrates this idea in Python: 
> https://pymotw.com/2/socket/multicast.html.


Well, I think it kinds of violates the multicast overlay here. It allows 
to receive any other traffic (unitcast) that may be received for the 
port which is not what we want here.

Thanks


>
>
> Regards,
> Faisal Al-Humaimidi
>
> On Mon., Feb. 17, 2020, 1:54 a.m. Jason Wang, <jasowang@redhat.com 
> <mailto:jasowang@redhat.com>> wrote:
>
>
>     On 2020/2/15 下午6:39, Markus Armbruster wrote:
>     > Jason, please have a look.
>     >
>     > Faisal Al-Humaimidi <falhumai96@gmail.com
>     <mailto:falhumai96@gmail.com>> writes:
>     >
>     >> Hello QEMU developers,
>     >>
>     >> I have noticed a bug in the `mcast` option of the `socket`
>     networking
>     >> backend, where I simply cannot join a multicast group (tested
>     in Windows 10
>     >> with QEMU 4.2.0 release). I have found a fix to the problem.
>     The problem
>     >> was mainly due to the fact that QEMU was binding to the
>     multicast address,
>     >> and not the local address or the default INADDR_ANY (0.0.0.0)
>     if no local
>     >> address is used.
>     >>
>     >> Here's the patch text (as well as attached with this email),
>     that outlines
>     >> my fix:
>     >>
>     >> ```
>     >> diff -uarN qemu-4.2.0.original/net/socket.c
>     qemu-4.2.0.modified/net/socket.c
>     >> --- qemu-4.2.0.original/net/socket.c 2019-12-12
>     10:20:48.000000000 -0800
>     >> +++ qemu-4.2.0.modified/net/socket.c 2020-02-14
>     10:30:16.395973453 -0800
>     >> @@ -253,6 +253,15 @@
>     >>           goto fail;
>     >>       }
>     >>
>     >> +    /* Preserve the multicast address, and bind to a
>     non-multicast group
>     >> (e.g. a
>     >> +     * local address).
>     >> +     */
>     >> +    struct in_addr group_addr = mcastaddr->sin_addr;
>     >> +    if (localaddr) {
>     >> +        mcastaddr->sin_addr = *localaddr;
>     >> +    } else {
>     >> +        mcastaddr->sin_addr.s_addr = htonl(INADDR_ANY);
>     >> +    }
>     >>       ret = bind(fd, (struct sockaddr *)mcastaddr,
>     sizeof(*mcastaddr));
>
>
>     This looks wrong, AFAIK the local address should be added through
>     IP_ADD_MEMBERSHIP which is already handled in this function I believe.
>
>     Thanks
>
>
>     >>       if (ret < 0) {
>     >>           error_setg_errno(errp, errno, "can't bind ip=%s to
>     socket",
>     >> @@ -260,7 +269,10 @@
>     >>           goto fail;
>     >>       }
>     >>
>     >> -    /* Add host to multicast group */
>     >> +    /* Restore the multicast address. */
>     >> +    mcastaddr->sin_addr = group_addr;
>     >> +
>     >> +    /* Add host to multicast group. */
>     >>       imr.imr_multiaddr = mcastaddr->sin_addr;
>     >>       if (localaddr) {
>     >>           imr.imr_interface = *localaddr;
>     >> @@ -277,7 +289,7 @@
>     >>           goto fail;
>     >>       }
>     >>
>     >> -    /* Force mcast msgs to loopback (eg. several QEMUs in same
>     host */
>     >> +    /* Force mcast msgs to loopback (eg. several QEMUs in same
>     host). */
>     >>       loop = 1;
>     >>       ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_LOOP,
>     >>                             &loop, sizeof(loop));
>     >> @@ -287,7 +299,7 @@
>     >>           goto fail;
>     >>       }
>     >>
>     >> -    /* If a bind address is given, only send packets from that
>     address */
>     >> +    /* If a bind address is given, only send packets from that
>     address. */
>     >>       if (localaddr != NULL) {
>     >>           ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_IF,
>     >>                                 localaddr, sizeof(*localaddr));
>     >> ```
>     >>
>     >> Regards,
>     >> Faisal Al-Humaimidi
>     >
>