|
Description
Yonit Halperin
2012-01-08 03:45:28 UTC
Created attachment 55343 [details] [review] gio-coroutine: hide g_condition_wait_source, use GLib style convention Created attachment 55344 [details] [review] Create a GCoroutine, get rid of wait_queue Created attachment 55345 [details] [review] Remove the non-interruptible version g_io_wait() Use the common g_coroutine_socket_wait() Created attachment 55346 [details] [review] Make g_coroutine_condition_wait() cancellable Created attachment 55347 [details] [review] Log if condition wait got cancelled Created attachment 55348 [details] [review] wait for cached images that haven't been added to the cache yet Comment on attachment 55348 [details] [review] wait for cached images that haven't been added to the cache yet Review of attachment 55348 [details] [review]: ----------------------------------------------------------------- ::: gtk/channel-display.c @@ +438,5 @@ > + .id = id, > + .image = NULL > + }; > + if (!g_coroutine_condition_wait(g_coroutine_self(), wait_image, &wait)) > + SPICE_DEBUG("wait lossless got cancelled"); I need to update that log Comment on attachment 55343 [details] [review] gio-coroutine: hide g_condition_wait_source, use GLib style convention Review of attachment 55343 [details] [review]: ----------------------------------------------------------------- Ack Comment on attachment 55344 [details] [review] Create a GCoroutine, get rid of wait_queue Review of attachment 55344 [details] [review]: ----------------------------------------------------------------- Ack + small comments ::: gtk/gio-coroutine.c @@ +64,2 @@ > g_return_val_if_fail(sock != NULL, 0); > add g_return_val_if_fail(&self->coroutine == coroutine_self) ? @@ +80,4 @@ > return *ret; > } > > +void g_io_wakeup(GCoroutine *self) "self" is a bit confusing here Comment on attachment 55345 [details] [review] Remove the non-interruptible version g_io_wait() Review of attachment 55345 [details] [review]: ----------------------------------------------------------------- Hi, I have a question that is related to this patch and the next patch as well: Do we always want to cancel waiting when wakeup is called? We surely want to cancel waiting when we destroy the session, but there is also a wakup call on spice-channel.spice_msg_out_send, which I'm not sure if we want it to cancel waiting. Comment on attachment 55347 [details] [review] Log if condition wait got cancelled Review of attachment 55347 [details] [review]: ----------------------------------------------------------------- ::: gtk/spice-channel.c @@ +1867,5 @@ > GIOCondition ret; > > do { > + if (!g_coroutine_condition_wait(&c->coroutine, wait_migration, channel)) > + SPICE_DEBUG("migration wait cancelled"); was "if (c->state == SPICE_CHANNEL_STATE_MIGRATING)" removed intentionally? Comment on attachment 55347 [details] [review] Log if condition wait got cancelled Review of attachment 55347 [details] [review]: ----------------------------------------------------------------- Ignore the previous comment. Ack Comment on attachment 55344 [details] [review] Create a GCoroutine, get rid of wait_queue Review of attachment 55344 [details] [review]: ----------------------------------------------------------------- ::: gtk/gio-coroutine.c @@ +64,2 @@ > g_return_val_if_fail(sock != NULL, 0); > I think it's a sane assumption. I just thought that technically there is no limitation in managing/scheduling another coroutine, if you know what you are doing... @@ +80,4 @@ > return *ret; > } > > +void g_io_wakeup(GCoroutine *self) In the header, we prefer using the name of the object (coroutine here), as "self" is more an implementation detail for current object. "self" doesn't really exist outside of the method. that's more by convention.. Comment on attachment 55345 [details] [review] Remove the non-interruptible version g_io_wait() Review of attachment 55345 [details] [review]: ----------------------------------------------------------------- Yeah, I think we want to be a bit more careful here. So I propose adding g_coroutine_condition_cancel() to cancel the condition, complementary to g_coroutine_wakeup() which will switch to specified coroutine. ok with that? Created attachment 55512 [details] [review] Lower connection error from warning to debug, it's normal to fail We try several connections, so it's normal to fail for some. No need to warn. Created attachment 55513 [details] [review] Hide g_condition_wait_source, use GLib style convention Created attachment 55514 [details] [review] Create a GCoroutine, get rid of wait_queue Created attachment 55515 [details] [review] Remove the non-interruptible version g_io_wait() Use the common g_coroutine_socket_wait() Created attachment 55516 [details] [review] Make g_coroutine_condition_wait() cancellable Created attachment 55517 [details] [review] Log if condition wait got cancelled Created attachment 55518 [details] [review] wait for cached images that haven't been added to the cache yet Comment on attachment 55512 [details] [review] Lower connection error from warning to debug, it's normal to fail Review of attachment 55512 [details] [review]: ----------------------------------------------------------------- Ack. Comment on attachment 55513 [details] [review] Hide g_condition_wait_source, use GLib style convention Review of attachment 55513 [details] [review]: ----------------------------------------------------------------- Ack. Comment on attachment 55514 [details] [review] Create a GCoroutine, get rid of wait_queue Review of attachment 55514 [details] [review]: ----------------------------------------------------------------- Ack. Comment on attachment 55515 [details] [review] Remove the non-interruptible version g_io_wait() Review of attachment 55515 [details] [review]: ----------------------------------------------------------------- Ack. Comment on attachment 55516 [details] [review] Make g_coroutine_condition_wait() cancellable Review of attachment 55516 [details] [review]: ----------------------------------------------------------------- Ack. Comment on attachment 55517 [details] [review] Log if condition wait got cancelled Review of attachment 55517 [details] [review]: ----------------------------------------------------------------- Ack. Comment on attachment 55518 [details] [review] wait for cached images that haven't been added to the cache yet Review of attachment 55518 [details] [review]: ----------------------------------------------------------------- Ack. all pushed in v0.8 |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.