[spice-gtk] spice-session: Fix SWAP_STR macro

Submitted by Frediano Ziglio on Sept. 6, 2019, 3:27 p.m.

Details

Message ID 20190906152704.5515-1-fziglio@redhat.com
State Accepted
Headers show
Series "spice-session: Fix SWAP_STR macro" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Sept. 6, 2019, 3:27 p.m.
Really swap "x" and "y", not temporary copies.
The issue was introduced by 01c6343 "Use macro to swap
data in spice_session_start_migrating()".

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 src/spice-session.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Removed RFC.
Tested, the original session is updated with the new values
after all connections are established.
As usually there are no other connection after these the
problem is not noted.

Patch hide | download patch | download mbox

diff --git a/src/spice-session.c b/src/spice-session.c
index 04ba124a..d0d9e541 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -1742,12 +1742,9 @@  void spice_session_switching_disconnect(SpiceSession *self)
 }
 
 #define SWAP_STR(x, y) G_STMT_START { \
-    const gchar *tmp;                 \
-    const gchar *a = x;               \
-    const gchar *b = y;               \
-    tmp = a;                          \
-    a = b;                            \
-    b = tmp;                          \
+    gchar *tmp = x;                   \
+    x = y;                            \
+    y = tmp;                          \
 } G_STMT_END
 
 G_GNUC_INTERNAL

Comments

> 
> Really swap "x" and "y", not temporary copies.
> The issue was introduced by 01c6343 "Use macro to swap
> data in spice_session_start_migrating()".
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  src/spice-session.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Removed RFC.
> Tested, the original session is updated with the new values
> after all connections are established.
> As usually there are no other connection after these the
> problem is not noted.
> 

The patch is working. I manage to test it setting up 2 VMs with iSCSI target,
quite easy setup not requiring ages to do it, with NFS I had an issue and
system was having problems shutting down.

By the way. The "swap" is quite weird, looks like an hack to avoid changing
SpiceSession (also also similar code for SpiceChannel) pointers but on
the other side it make the code harder to understand. For instance for channel
is more complicated, you copy the base SpiceChannel (see the problem of "object
slicing" in C++) but then you have to reset the channel in order to put the
reset of the state to a proper one. On the server, IMHO more correctly, new
channel objects (also because the old object are in another machine!) replace
old ones.

Another problem that I noted is at protocol level. The message from main channel
(DstInfo basically) contains new:
- hostname
- port
- secure port
- certificate
This however does not cover:
- accesses through ssh
- accesses through proxy
- using unix socket (if one host is local)
Maybe would be worth adding a generic URL? Note that in case of proxy the
URL should be provided externally (maybe depending on the client).

I remember also Victor had some additional need to extend the protocol
due to other migration limitations.

> diff --git a/src/spice-session.c b/src/spice-session.c
> index 04ba124a..d0d9e541 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -1742,12 +1742,9 @@ void spice_session_switching_disconnect(SpiceSession
> *self)
>  }
>  
>  #define SWAP_STR(x, y) G_STMT_START { \
> -    const gchar *tmp;                 \
> -    const gchar *a = x;               \
> -    const gchar *b = y;               \
> -    tmp = a;                          \
> -    a = b;                            \
> -    b = tmp;                          \
> +    gchar *tmp = x;                   \
> +    x = y;                            \
> +    y = tmp;                          \
>  } G_STMT_END
>  
>  G_GNUC_INTERNAL
> --
> 2.20.1
> 
>