3:12 a.m.
See 2/2 for explanation.
BUT, I am not fully convinced about changing XDR though. In my testing
it did not really show to be that backward compatible. I did the
following tests:
1) run libvirtd unpatched,
2) run libvirtd with just arguments if the callback fixed (i.e. just the
first hunk of 2/2),
3) run libvirtd with patch 2/2 as is
And also, I repeated the same for split daemon using qemu:///system and
network:///system URIs. I've tested the following command:
virsh net-update default add dns-txt '<txt name="example"
value="example value"/>'
and if I got the following error message:
Operation not supported: can't update 'ip' section of network
'default'
I knew that arguments are swapped (because 'add' command has value 3,
'dns-txt' section has value 11 and 'ip' section has value 3).
From the results I've constructed the following tables:
| libvirtd (case 1) | libvirtd + swap (case 2) | libvirtd + RPC (case 3)
--------------+-------------------+--------------------------+------------------------
client | 1 | 0 | 0
client + swap | 0 | 1 | 1
client + RPC | 0 | 1 | 1
Both client and libvirtd were tested for all three cases as described
above; 1 means success (API returned without an error), 0 means the
error message from above was observed. For libvirtd I've tested just
qemu:///system, because network:///system has to yield the same results.
For split daemon (SD) case:
qemu:///system
| SD (case 1) | SD + swap (case 2) | SD + RPC (case 3)
--------------+-------------+--------------------+------------------
client | 0 | 0 | 0
client + swap | 1 | 1 | 1
client + RPC | 1 | 1 | 1
network:///system
| SD (case 1) | SD + swap (case 2) | SD + RPC (case 3)
--------------+-------------+--------------------+------------------
client | 1 | 0 | 0
client + swap | 0 | 1 | 1
client + RPC | 0 | 1 | 1
Therefore, what I am trying to say is, there is no case where plain swap
(= case 2) would not help and including RPC change (= case 3) would
help. Maybe I'm missing something?
Michal Prívozník (2):
lib: Debug print all arguments of virNetworkUpdate()
lib: Fix calling of virNetworkUpdate() driver callback
src/libvirt-network.c | 6 +++---
src/remote/remote_protocol.x | 7 ++++++-
src/remote_protocol-structs | 2 +-
3 files changed, 10 insertions(+), 5 deletions(-)
--
2.26.2
3:12 a.m.
New subject: [PATCH 1/2] lib: Debug print all arguments of virNetworkUpdate()
Somehow, command argument was not printed into debug logs. It is
imperative that all arguments are logged.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt-network.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/libvirt-network.c b/src/libvirt-network.c
index 92bdbacbff..b84389f762 100644
--- a/src/libvirt-network.c
+++ b/src/libvirt-network.c
@@ -530,8 +530,8 @@ virNetworkUpdate(virNetworkPtr network,
unsigned int flags)
{
virConnectPtr conn;
- VIR_DEBUG("network=%p, section=%d, parentIndex=%d, xml=%s, flags=0x%x",
- network, section, parentIndex, xml, flags);
+ VIR_DEBUG("network=%p, command=%d, section=%d, parentIndex=%d, xml=%s,
flags=0x%x",
+ network, command, section, parentIndex, xml, flags);
virResetLastError();
--
2.26.2
6:31 p.m.
New subject: [PATCH 1/2] lib: Debug print all arguments of virNetworkUpdate()
On a Tuesday in 2021, Michal Privoznik wrote:
Somehow, command argument was not printed into debug logs. It is
imperative that all arguments are logged.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt-network.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
3:12 a.m.
New subject: [PATCH 2/2] lib: Fix calling of virNetworkUpdate() driver callback
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). 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(a)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);
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. */
struct remote_network_update_args {
remote_nonnull_network net;
- unsigned int command;
unsigned int section;
+ unsigned int command;
int parentIndex;
remote_nonnull_string xml;
unsigned int flags;
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index c0c034ac6a..800678c92e 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -1109,8 +1109,8 @@ struct remote_network_undefine_args {
};
struct remote_network_update_args {
remote_nonnull_network net;
- u_int command;
u_int section;
+ u_int command;
int parentIndex;
remote_nonnull_string xml;
u_int flags;
--
2.26.2
8:34 p.m.
New subject: [PATCH 2/2] lib: Fix calling of virNetworkUpdate() driver callback
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(a)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)
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.
Jano
struct remote_network_update_args {
remote_nonnull_network net;
- unsigned int command;
unsigned int section;
+ unsigned int command;
int parentIndex;
remote_nonnull_string xml;
unsigned int flags;
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index c0c034ac6a..800678c92e 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -1109,8 +1109,8 @@ struct remote_network_undefine_args {
};
struct remote_network_update_args {
remote_nonnull_network net;
- u_int command;
u_int section;
+ u_int command;
int parentIndex;
remote_nonnull_string xml;
u_int flags;
--
2.26.2
3:45 a.m.
New subject: [PATCH 2/2] lib: Fix calling of virNetworkUpdate() driver callback
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(a)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
1383
days inactive
1397
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik