On 11/26/2015 02:34 PM, Frediano Ziglio wrote:
On 11/25/2015 02:23 PM, Frediano Ziglio wrote:
On Mon, 2015-11-23 at 17:01 +0000, Frediano Ziglio wrote:
From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
As I said in my previous review, I think that using g_return_if_fail()
here
is
appropriate.
Yes, actually the only caller ignore the result so false or true is the
same.
Also there is the same check outside so this path is never taken.
Basically the code is handling messages for migration till migration is
ended.
red_channel_client_waits_for_migrate_data just returns if we are in a
migration
(TRUE) or not (FALSE).
---
server/display-channel.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/server/display-channel.c b/server/display-channel.c
index 5e75019..a178cc9 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -861,10 +861,10 @@ int
display_channel_wait_for_migrate_data(DisplayChannel
*display)
RedChannelClient *rcc;
spice_debug(NULL);
- spice_assert(channel->clients_num == 1);
+ spice_warn_if_fail(channel->clients_num == 1);
I agree with this warning but here I'd like to see a TODO also.
I won't kill the VM just because we have a single client.
This line verifies that there is only a single client.
In other words, that migration with multi-client is not-supported,
and also if we got here there must be a client connected.
I don't think is so easy here to handle multiple clients and by the
way is not supported.
I agree, the code does not handle it yet.
The reason for the warning...
rcc = SPICE_CONTAINEROF(ring_get_head(&channel->clients),
RedChannelClient, channel_link);
Is this! Only the first client is considered!
The first and only client is considered.
regards,
Uri.
Is this a NACK or not?
This is an example for the return/assert discussion.
We should be careful with simply replacing one with the other.
I'm pointing out that with the assert it's clear what would happen
if e.g. 2 (or more) clients are connected. But it's not easy to see what
happens in such a scenario with _return_if or _warn_if.
You too mentioned there may be a problem with multiple clients
and that the code will only handle the first client (since
it's assumed that there can only be a single client after
the assert).
The multi-client feature is experimental anyway.
In case of num_clients==0, ring_get_head returns NULL and the
server crashes soon after.
Also as you mentioned, both those checks are checked before
calling this function in the single caller of this function
so currently both checks are irrelevant.
This is not NACK, but Ido not mind dropping this patch.
At least a better commit log explaining why it's safe to replace
those two asserts is needed.
Regards,
Uri.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel