[PATCH 2/2] lib: Fix calling of virNetworkUpdate() driver callback

[Michal Privoznik]

On 3/16/21 2:34 AM, Ján Tomko wrote:
> On a Tuesday in 2021, Michal Privoznik wrote:
>> Let me take you ~8 years back. Back then, virNetworkUpdate() API
>> was introduced and the public implementation is nothing special -
>> it calls the networkUpdate() callback of the network driver.
>> Except, a small "typo" slipped through - while the public API
>> takes @command argument first followed by @section argument,
>> these are passed to the callback in swapped order. This wasn't
>> visible, until split daemons and sub-driver URIs became a thing.
>> Simply because the client was talking to the network driver via
>> our remote driver.
>>
>> On client side, when the public API was called the RPC layer
>> swapped the order (because it was called with swapped arguments
>> already). 
> 
> So it did not swap it, then? :)
> 
>> Then, on the server side, after deserialization the
>> public API was called again (still with swapped arguments) and it
>> subsequently called network driver callback (where the arguments
>> were in the right order again).
>>
>> Since both arguments are of the same type (unsigned int) XDR was
>> happy too.
>>
>> The problem arose with split daemons and sub-driver URIs. Imagine
>> a user running split daemons. When they connect to
>> network:///system, they talk to virnetworkd "directly" (as in no
>> proxy daemon sits in between). Both sides still use remote driver
>> though, so the order is fixed by RPC layer, just like in libvirtd
>> case. Connecting to qemu:///system is where things get
>> interesting because virtqemud serves as a proxy to virtnetworkd
>> (the former talks to the later on users behalf and forwards API
>> calls).
>>
>> What happens is, virtqemud sees incoming RPC packet (with swapped
>> arguments), it decodes it and calls remoteDispatchNetworkUpdate()
>> (valued are still swapped in remote_network_update_args struct).
>> Subsequently, virNetworkUpdate() is called and since virtqemud
>> has no network driver, it calls remote driver again. But this
>> time, the API callback gets the arguments in correct order (just
>> like network driver would in case of libvirtd). But this means,
>> that virnetworkd will see them in wrong order, because it swaps
>> them again.
>>
>> After fixing the call of the callback, the API works again in
>> both cases, if both sides run with the fix.
>>
>> And to make things work for newer clients talking to older
>> servers, we have to swap the order on RPC layer too. If both
>> sides run with the change, they both encode and decode packet
>> properly. But if newer client talks to older server, it will
>> encode packet just how the older server expects it.
>>
>> Fixes: 574b9bc66b6b10cc4cf50f299c3f0ff55f2cbefb
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1870552
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/libvirt-network.c        | 2 +-
>> src/remote/remote_protocol.x | 7 ++++++-
>> src/remote_protocol-structs  | 2 +-
>> 3 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libvirt-network.c b/src/libvirt-network.c
>> index b84389f762..310da44a4e 100644
>> --- a/src/libvirt-network.c
>> +++ b/src/libvirt-network.c
>> @@ -543,7 +543,7 @@ virNetworkUpdate(virNetworkPtr network,
>>
>>     if (conn->networkDriver && conn->networkDriver->networkUpdate) {
>>         int ret;
>> -        ret = conn->networkDriver->networkUpdate(network, section, 
>> command,
>> +        ret = conn->networkDriver->networkUpdate(network, command, 
>> section,
>>                                                  parentIndex, xml, 
>> flags);
> 
> Especially in the migration code, we use VIR_DRV_SUPPORTS_FEATURE to
> figure out whether the remote side has some functionality.
> 
> With something like VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER,
> we could pass the correct order if it's safe to do so and fix the
> virtqemud use case for newer daemons. (Since it never worked, I think
> it's nicer to not fix it for older virtqemud than to break client
> compatibility with older libvirt)

Good idea. Let me try and see where it leads.

> 
>>         if (ret < 0)
>>             goto error;
>> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
>> index d3724bc305..464c6b4af1 100644
>> --- a/src/remote/remote_protocol.x
>> +++ b/src/remote/remote_protocol.x
>> @@ -1542,10 +1542,15 @@ struct remote_network_undefine_args {
>>     remote_nonnull_network net;
>> };
>>
>> +/* The @section and @command members are intentionally inverted 
>> compared to the
>> + * virNetworkUpdate() API. The reason is that since it's introduction 
>> until the
>> + * 7.2.0 release the driver callback was given arguments in inverted 
>> order.
>> + * After it was fixed, the XDR has to be swapped to keep 
>> compatibility with
>> + * older daemons. */
> 
> This does not actually affect RPC at all, merely the names used in the
> autogenerated stubs. If it did, I'm afraid it would just move the bug
> from the driver to the RPC layer.

Yes, that's what I wrote in the cover letter too.

Michal