> 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 ? > I think so. > > > > 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. > I sent some patches. > > - 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 ? > Yes. > > - 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. > Sent a patch event for this. > I think you are right. > > Thanks, > Uri. > > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel