On 12/02/2015 02:02 PM, Frediano Ziglio wrote:
On 11/27/2015 11:54 AM, Frediano Ziglio wrote:
What about:
display: move checks inside display_channel_wait_for_migrate_data
Instead of relaying on the caller to use red_channel_waits_for_migrate_data
to check if there are client waiting for migration data check inside the
function. This make sure that the check is done so some check can be
removed
from the function.
Frediano
Hi Frediano,
In this specific case the asserts' conditions were already tested by the
(only) calling function. If someone decides to call this function
without verifying those conditions that would be an error we want
to discover early. Them being there also act as a hint for the reader
that those conditions are assumed to be true (although that's
also true for spice_return_if lines).
Your solution works, as it moves the responsibility of the checks
within the function. Although you may still be inclined to verify
(assert) that those conditions are true before proceeding.
Personally I do not mind having those asserts in the code.
If people prefer, we can also return_if_fail and let the
calling functions deal with failures (which it currently
does not, as it checks those conditions itself).
So in the patch we can replace spice_warn_if_fail with
spice_return_val_if_fail.
Also add a commit message saying that those checks
are being tested by the calling function and that the calling
function needs to handle failures.
Hi Uri,
with last version of patch the caller does not have to do nothing
as the call to red_channel_waits_for_migrate_data was moved inside
display_channel_wait_for_migrate_data. This make sure the state
is as expected. This also assure there is at least one client
connected. So
spice_warn_if_fail(channel->clients_num == 1);
OK.
Is spice_warn better than spice_assert here ?
is just checking that there are more clients as the loop does not
handle this case.
Regards,
Uri.
OT: the more look at this patch the more I don't like this function:
- red_channel_waits_for_migrate_data should be red_channel_is_waiting_for_migrate_data;
I agree.
- display_channel_wait_for_migrate_data should be display_channel_handle_migrate_data;
OK, although it both waits (by sleeping) and handles.
- code should use loop code and not do polling and relaying on red_channel_client_receive
being not blocking. This is actually true for all usleep(DISPLAY_CLIENT_RETRY_INTERVAL)
calls;
I'm not sure what you mean. Do you mean using events/callbacks
instead of polling/sleeping ?
- there is the wrong assumption that pointers got from channel->clients are safe
to survive all data processing but this is not true. We should increment the
reference before the loop and decrement before exiting the function.
I think you are right.
Thanks,
Uri.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel