> > 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); 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; - display_channel_wait_for_migrate_data should be display_channel_handle_migrate_data; - 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; - 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. Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel