GNOME Bugzilla – Bug 747412
port to GTask
Last modified: 2017-11-28 13:54:26 UTC
GSimpleAsyncResult is now marked deprecated in git master. Way back when I first wrote GTask, I had started porting gvfs, as a test case, but I never finished, and never tested it. But anyway, I've pushed that work to wip/gtask-porting now. (This isn't particularly urgent if you don't care about the deprecation warnings in the build. I just wanted to point out that branch so if someone does want to fix this, they can use that as a starting point rather than starting over from scratch.)
Yes, the port will be necessary. Thanks for the branch, it is useful. Unfortunately it involves pretty much changes and lot of your work isn't already applicable to master. I've already realized that you use e.g. g_task_async_result_complete, which isn't part of the api (probably you just automatically replaced g_simple -> g_task and didn't finish rewriting), so also applicable code needs to be fixed...
Created attachment 303412 [details] [review] gdaemonfile: Port to GTask
Created attachment 303413 [details] [review] gvfsbackend: Port to GTask
Review of attachment 303412 [details] [review]: ::: client/gdaemonfile.c @@ +571,3 @@ { + g_dbus_error_strip_remote_error (error); + g_task_return_error (data->task, error); + async_proxy_create_free (data); @@ +623,3 @@ { + g_dbus_error_strip_remote_error (error); + g_task_return_error (data->task, error); + async_proxy_create_free (data); @@ +661,3 @@ { g_dbus_error_strip_remote_error (error); + g_task_return_error (data->task, error); + async_proxy_create_free (data);
Created attachment 304242 [details] [review] gdaemonmount: Port to GTask
Created attachment 304243 [details] [review] gvfsiconloadable: Port to GTask
See wip/oholy/gtask for uptodate versions of patches...
It would be nice to port GVfs to use GTask... to avoid tons of deprecated warnings. So I added some more patches into wip/oholy/gtask. Can somebody take a look at them, please?
Created attachment 336866 [details] [review] client: Port GDaemonFile to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced. Based on patch from Dan Winship.
Created attachment 336867 [details] [review] client: Port GDaemonMount to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced. Based on patch from Dan Winship.
Created attachment 336868 [details] [review] client: Port GVfsIconLoadable to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced. This commit also slightly changes API to be more similiar with gdeamonfile. Based on patch from Dan Winship.
Created attachment 336869 [details] [review] client: Port GDaemonFileEnumerator to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336870 [details] [review] client: Port GDaemonFileInputStream to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336871 [details] [review] client: Port GDaemonFileOutputStream to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336872 [details] [review] client: Port GVfsDaemonDBus to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336873 [details] [review] common: Port GMountSource to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336874 [details] [review] common: Port GVfsDnsSdResolver to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336875 [details] [review] common: Port GVfsMountInfo to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336876 [details] [review] daemon: Port GVfsBackend to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced. This patch also introduce g_vfs_backend_register_mount_finish() and g_vfs_backend_unregister_mount_finish() functions to make the code nicer. Based on patch from Dan Winship.
Created attachment 336877 [details] [review] udisks2: Port GVfsUDisks2Drive to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336878 [details] [review] udisks2: Port GVfsUDisks2Volume to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336879 [details] [review] udisks2: Port GVfsUDisks2Utils to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336880 [details] [review] udisks2: Port GVfsUDisks2Mount to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336881 [details] [review] goa: Port GVfsGoaVolume to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336882 [details] [review] proxy: Port GProxyVolume to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336883 [details] [review] proxy: Port GProxyMount to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336884 [details] [review] proxy: Port GProxyDrive to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336885 [details] [review] afp: Port GVfsBackendAfpBrowse to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336886 [details] [review] afp: Port GVfsAfpServer to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336887 [details] [review] afp: Port GVfsAfpConnection to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 336888 [details] [review] afp: Port GVfsAfpVolume to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
The new development phase begins, so I rebased the patches on master and attached them for easier reviewing. It would be nice to push them in the near future to have enough time for fixing eventual issues. I've tested moreless overall functionality and it seems it works properly, however, AFP patches are untested (I haven't setup AFP server yet)...
Review of attachment 336881 [details] [review]: This looks very good, Ondrej. I found some minor issues regarding the lifetime of the GTask. ::: monitor/goa/goavolume.c @@ +137,3 @@ if (self->mount == NULL) + { + g_task_return_error (task, error); We are leaking the GTask here. We have to unref it after any g_task_return_* call. Earlier, with GSimpleAsyncResult we had our own wrapper callback which took care of dropping the reference on the GSimpleAsyncResult and the MountOp. Since, the wrapper callback is no longer needed with GTask (because we have g_task_set_task_data), we now need to unref the task when 'returning'. @@ +142,3 @@ + { + g_signal_connect (self->mount, "unmounted", G_CALLBACK (mount_unmounted_cb), self); + g_task_return_boolean (task, TRUE); Ditto. (re: leaking the GTask) @@ +158,3 @@ if (error->code != G_IO_ERROR_ALREADY_MOUNTED) { + g_task_return_error (task, error); Ditto. (re: leaking the GTask) @@ -189,3 @@ } - g_file_find_enclosing_mount_async (root, G_PRIORITY_DEFAULT, NULL, find_enclosing_mount_cb, simple); I wonder why we used a NULL GCancellable here. It was probably a mistake. @@ +193,3 @@ if (!goa_oauth2_based_call_get_access_token_finish (oauth2_based, &data->passwd, NULL, res, &error)) { + g_task_return_error (task, error); Ditto. (re: leaking the GTask) @@ +203,3 @@ + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, + _("Failed to get org.gnome.OnlineAccounts.Files for %s"), + goa_account_get_id (account)); Ditto. (re: leaking the GTask) @@ +230,3 @@ if (!goa_password_based_call_get_password_finish (passwd_based, &data->passwd, res, &error)) { + g_task_return_error (task, error); Ditto. (re: leaking the GTask) @@ +240,3 @@ + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, + _("Failed to get org.gnome.OnlineAccounts.Files for %s"), + goa_account_get_id (account)); Ditto. (re: leaking the GTask) @@ +270,3 @@ + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, + _("Invalid credentials for %s"), + goa_account_get_presentation_identity (account)); Ditto. (re: leaking the GTask) @@ +274,3 @@ } else + g_task_return_error (task, error); Ditto. (re: leaking the GTask) @@ +302,3 @@ + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED, + _("Unsupported authentication method for %s"), + goa_account_get_presentation_identity (account)); Ditto. (re: leaking the GTask) @@ +416,2 @@ data = g_slice_new0 (MountOp); + task = g_task_new (G_OBJECT (self), cancellable, callback, user_data); Nitpick: (a) The first argument is a gpointer, not a GObject *. So there is no need for the G_OBJECT cast. (b) The source_tag check needs to be done explicitily with g_task_set_source_tag, but maybe that's not so important. @@ +416,3 @@ data = g_slice_new0 (MountOp); + task = g_task_new (G_OBJECT (self), cancellable, callback, user_data); + g_task_set_task_data (task, data, (GDestroyNotify)mount_op_free); Nitpick: missing white space after the closing parenthesis.
Created attachment 336896 [details] [review] goa: Port GVfsGoaVolume to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Thanks for the review! Good catch with the task leaks, however, it should be ok in the other patches. I've added also the source_tag check, although I am not sure it is so important.
Review of attachment 336896 [details] [review]: Nice. This looks good to me.
Review of attachment 336873 [details] [review]: I never worked with this part of GVfs, so I can't claim to review it. I took a look merely to satisfy my curiosity. :) ::: common/gmountsource.c @@ +627,3 @@ gint *choice_out) { + AskQuestionData *data, def = { TRUE, }; Wasn't this a bug in its own right? It does seem to change the behaviour a bit. Earlier: If in ask_question_reply, gvfs_dbus_mount_operation_call_ask_question_finish returned FALSE, then data->aborted == TRUE and an error was set. However, in g_mount_source_ask_question_finish, def == {FALSE, } (ie. def->aborted == FALSE), and we were using it as data when an error was set. That means that the value of 'aborted' that we were returning to the callback was getting inverted from TRUE to FALSE. We were also setting 'aborted' to FALSE for handled == FALSE. Now we set it to TRUE. This makes me think that this was a bug that should be fixed/backported independent of this GTask patch series. Or am I missing something? Or maybe this doesn't cause problems in practice? In that case, sorry for the noise. :)
I see that we weren't using g_simple_async_result_set_check_cancellable at all in the past, which is something that GTask does by default. This means that we are implicitly fixing all the 'should set G_IO_ERROR_CANCELLED when cancelled' bugs that nobody noticed so far. :)
Thanks for your review again. I prepared these patches for many parts which I had never touched before. So, feel free to review the patches, we are on the same board :-) I'm also convinced that this was a bug... and I forgot to split this patch. However, I don't think it will cause problems somewhere, but worth to check... I will do something with it.
Review of attachment 336874 [details] [review]: Looks good apart from a small reference counting problem: ::: common/gvfsdnssdresolver.c @@ +1089,3 @@ } + + g_object_unref (task); We have to move the unref inside the branches where we actually call g_task_return_* because we don't want to drop the reference for the 'data->resolver->address != NULL' case. @@ +1097,3 @@ + ResolveData *data = g_task_get_task_data (task); + + data->timeout_id = 0; Nitpick: it didn't have to be moved, but OK. :) @@ +1132,1 @@ return FALSE; We need to unref the GTask here. @@ +1171,3 @@ + task); + + g_task_set_task_data (task, data, (GDestroyNotify)resolve_data_free); Nitpick: missing white space after the closing parenthesis.
Created attachment 337296 [details] [review] common: Port GVfsDnsSdResolver to GTask GSimpleAsyncResult is deprecated in favour of GTask and should be replaced.
Created attachment 337300 [details] [review] common: Port GVfsDnsSdResolver to GTask
Created attachment 337301 [details] [review] gmountsource: Return "aborted" flag consistently "aborted" flag is not returned consistently from the following functions if the async reply containes an error: g_mount_source_show_processes, g_mount_source_ask_question, and g_mount_source_ask_password. Set "aborted" always on TRUE if an error occurs. This change should not affect current functionality, because the usual workflow is the following: if (!g_mount_source_ask_password (..., &aborted, ...) || aborted) ...
Created attachment 337302 [details] [review] common: Port GMountSource to GTask
Just a note that I added also source tag checks in attachment 337300 [details] [review] and attachment 337302 [details] [review].
Review of attachment 337300 [details] [review]: This looks good to me.
(In reply to Ondrej Holy from comment #46) > Just a note that I added also source tag checks in attachment 337300 [details] [review] > [details] [review] and attachment 337302 [details] [review] [review]. Yes, I noticed. Great!
Review of attachment 337302 [details] [review]: Unfortunately, the source_tag introduced a complication: ::: common/gmountsource.c @@ +176,3 @@ { if (callback != NULL) + g_task_report_new_error (source, callback, user_data, NULL, Passing NULL as the source_tag is problematic because the *_finish methods won't like it. One option is to drop the source_tag checks. The other option is to pass the GTask, instead of callback and user_data, to create_mount_operation_proxy_sync and then use g_task_return_* instead of g_task_report_*. @@ +193,3 @@ + { + g_dbus_error_strip_remote_error (error); + g_task_report_error (source, callback, user_data, NULL, error); See above.
Review of attachment 337301 [details] [review]: Thanks for splitting this out, Ondrej. This looks more correct than the existing code. Although, I wonder if 'aborted' should be TRUE when 'handled' is TRUE.
Review of attachment 336867 [details] [review]: Looks good to me, other than a few minor style issues: ::: client/gdaemonmount.c @@ +349,1 @@ + g_task_set_task_data (task, data, (GDestroyNotify)async_proxy_create_free); Nitpick: missing white space after the closing parenthesis. @@ -359,3 @@ GError **error) { - return TRUE; Wow! The old code looks so wrong. We do throw an error sometimes. @@ +419,3 @@ + g_task_return_error (task, error); + else + g_task_return_pointer (task, type, (GDestroyNotify)g_strfreev); Nitpick: missing white space after the closing parenthesis.
Review of attachment 336868 [details] [review]: Looks good to me, apart from one nitpick: ::: client/gvfsiconloadable.c @@ +242,3 @@ GAsyncReadyCallback op_callback, gpointer op_callback_data, CreateProxyAsyncCallback callback, There are some stray tabs in these lines. Maybe something to be cleaned up later. @@ +340,3 @@ create_proxy_for_icon_async (G_VFS_ICON (icon), cancellable, callback, user_data, Just a thought. The way the older code was structured seems a bit odd to me. I would put the body of create_proxy_for_icon_async here and call load_async_cb from async_proxy_new_cb. But that's not relevant to this patch series, so it's fine. @@ +342,3 @@ callback, user_data, + load_async_cb, + data, (GDestroyNotify) g_free); Nitpick: g_free is already a GDestroyNotify - no need to explicitly cast it. @@ -355,3 @@ - op = g_simple_async_result_get_op_res_gpointer (simple); - if (op) - return g_object_ref (op); Wow! We were never setting the GError before.
Thanks for those reviews! (In reply to Debarshi Ray from comment #50) > Review of attachment 337301 [details] [review] [review]: > > Thanks for splitting this out, Ondrej. This looks more correct than the > existing code. Although, I wonder if 'aborted' should be TRUE when 'handled' > is TRUE. I am not sure I understand what you mean, but aborted should be TRUE in the two following cases and FALSE otherwise: - an error occurred, e.g. mount operation was not provided by user (handled == FALSE) - user aborted the operation, e.g. pressed "Cancel" (handled == TRUE)
(In reply to Debarshi Ray from comment #52) > Review of attachment 336868 [details] [review] [review]: > > Looks good to me, apart from one nitpick: > > ::: client/gvfsiconloadable.c > @@ +242,3 @@ > GAsyncReadyCallback op_callback, > gpointer op_callback_data, > CreateProxyAsyncCallback callback, > > There are some stray tabs in these lines. Maybe something to be cleaned up > later. Unfortunately, gvfs contains a lot of places with various white space issues. However, I don't like patches fixing just whitespace. I prefer fixing them just with code changes. I usually fix only "empty" lines in surrounding, but in this case, I think it makes sense to fix those lines together with those changes... > @@ +340,3 @@ > create_proxy_for_icon_async (G_VFS_ICON (icon), > cancellable, > callback, user_data, > > Just a thought. The way the older code was structured seems a bit odd to me. > I would put the body of create_proxy_for_icon_async here and call > load_async_cb from async_proxy_new_cb. But that's not relevant to this patch > series, so it's fine. You are right, it is unnecessarily complicated here because create_proxy_for_icon_async is used only once, but it is done in the same way as in gdeamonfile, where create_proxy_for_file is used several times... so I like this, although it is a bit unnoticed.
Created attachment 338500 [details] [review] common: Port GMountSource to GTask Fixed the source tags...
Created attachment 338503 [details] [review] client: Port GDaemonMount to GTask Fixed the nitpicks. Added source_tag checks.
Created attachment 338508 [details] [review] client: Port GVfsIconLoadable to GTask Fixed the nitpicks. Added source_tag checks.
Attachment 336896 [details] pushed as fdda284 - goa: Port GVfsGoaVolume to GTask Attachment 337300 [details] pushed as da9b232 - common: Port GVfsDnsSdResolver to GTask
Review of attachment 338500 [details] [review]: Looks good to me. ::: common/gmountsource.c @@ +174,3 @@ if (source->dbus_id[0] == 0) { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Internal Error"); Nice!
(In reply to Ondrej Holy from comment #54) > (In reply to Debarshi Ray from comment #52) > > Review of attachment 336868 [details] [review] [review] [review]: > > > > Looks good to me, apart from one nitpick: > > > > ::: client/gvfsiconloadable.c > > @@ +242,3 @@ > > GAsyncReadyCallback op_callback, > > gpointer op_callback_data, > > CreateProxyAsyncCallback callback, > > > > There are some stray tabs in these lines. Maybe something to be cleaned up > > later. > > Unfortunately, gvfs contains a lot of places with various white space > issues. However, I don't like patches fixing just whitespace. I prefer > fixing them just with code changes. I usually fix only "empty" lines in > surrounding, but in this case, I think it makes sense to fix those lines > together with those changes... Good point. Too many whitespace fixes damage the git history. Anyway, not a big deal and not really related to this patch series.
(In reply to Ondrej Holy from comment #53) > Thanks for those reviews! > > (In reply to Debarshi Ray from comment #50) > > Review of attachment 337301 [details] [review] [review] [review]: > > > > Thanks for splitting this out, Ondrej. This looks more correct than the > > existing code. Although, I wonder if 'aborted' should be TRUE when 'handled' > > is TRUE. > > I am not sure I understand what you mean, but aborted should be TRUE in the > two following cases and FALSE otherwise: > - an error occurred, e.g. mount operation was not provided by user (handled > == FALSE) > - user aborted the operation, e.g. pressed "Cancel" (handled == TRUE) We are always returning aborted = TRUE in the *_finish methods for (a) error due to aborted = TRUE, and (b) error due to handled = TRUE. I was wondering about (b). Either way, I think the change in attachment 337301 [details] [review] is an improvement over the old code, so this is not a blocker.
Review of attachment 338508 [details] [review]: Looks good to me.
Review of attachment 338503 [details] [review]: ++
Attachment 337301 [details] pushed as 6df182c - gmountsource: Return "aborted" flag consistently Attachment 338500 [details] pushed as c6b5d60 - common: Port GMountSource to GTask Attachment 338503 [details] pushed as d058784 - client: Port GDaemonMount to GTask Attachment 338508 [details] pushed as 811aace - client: Port GVfsIconLoadable to GTask
Review of attachment 336870 [details] [review]: ::: client/gdaemonfileinputstream.c @@ +1684,3 @@ { + g_task_return_new_error (iterator->task, G_IO_ERROR, G_IO_ERROR_FAILED, + _("Error in stream protocol: %s"), io_error->message); We should unref the GTask and free the AsyncIterator. @@ +1691,3 @@ { + g_task_return_new_error (iterator->task, G_IO_ERROR, G_IO_ERROR_FAILED, + _("Error in stream protocol: %s"), _("End of stream")); Same here. @@ +1841,1 @@ + g_task_return_int (task, count_read); This should be in an 'else' branch. @@ +1880,3 @@ GError **error) { + g_return_val_if_fail (g_task_is_valid (result, stream), FALSE); It should return -1, not FALSE, on error. I said -1 because that is the documented g_input_stream_read_finish behaviour: https://developer.gnome.org/gio/stable/GInputStream.html#g-input-stream-read-finish @@ +1937,1 @@ + g_task_return_boolean (task, TRUE); This should be in an 'else' branch. @@ +2025,3 @@ GError **error) { + g_return_val_if_fail (g_task_is_valid (result, stream), FALSE); NULL, not FALSE.
Comment on attachment 336871 [details] [review] client: Port GDaemonFileOutputStream to GTask Thanks for the review. It looks like I've pushed some work-in-progress version of this patch and lost the final one :-/ Same problems are in GDaemonFileOutputStream patch...
Created attachment 339049 [details] [review] client: Port GDaemonFileInputStream to GTask Updated per review. Source tag checks added.
Created attachment 339050 [details] [review] client: Port GDaemonFileOutputStream to GTask
Created attachment 339051 [details] [review] client: Remove obsolete GSimpleAsyncResult helpers
Review of attachment 339049 [details] [review]: Thanks, Ondrej. Looks good to me, apart from a tiny nitpick below. ::: client/gdaemonfileinputstream.c @@ +1984,2 @@ + info = op->info; + error = op->ret_error; One thing that bothers me is the ret_error member in the Close/Query/ReadOperation structs. There is nothing wrong at the moment, but given how the code winds its way through a bunch of structs and callbacks, it seems very easy to introduce a leak or something in a future commit. Basically, the code assumes that ret_error will only be set when the state is STATE_OP_DONE. The other states (ie. STATE_OP_READ/SKIP/CLOSE) will set their own errors through async_op_handle and there should never be two competing errors. I wish we asserted this (in a separate patch, of course) via g_assert or g_return_* or something similar. One option is to have a GDestroyNotify for all the structs, NULLify ret_error after g_task_return_error and assert that it is NULL in the GDestroyNotify. Anyway, just a thought. Feel free to ignore it. :) There is nothing with this patch. @@ +2015,3 @@ op->attributes = g_strdup (""); + g_task_set_task_data (task, op, (GDestroyNotify)query_operation_free); Nitpick: missing space after closing parenthesis.
Thanks for the review and for the note about ret_error, I will reconsider this, however, truly, I would rather not touching this code if it isn't really needed :-D Thanks for the nitpick. I was surprised by the previous reviews that space should be after closing parenthesis, because I wasn't used to doing it, but the previous files use really such style. However, this file doesn't use this style, so I will honor the style in the file :-)
Comment on attachment 339049 [details] [review] client: Port GDaemonFileInputStream to GTask Attachment 339049 [details] pushed as 3a5f04c - client: Port GDaemonFileInputStream to GTask
Created attachment 340900 [details] [review] client: Port GDaemonFile to GTask Added source tag checks...
Created attachment 340906 [details] [review] client: Port GDaemonFileEnumerator to GTask Added source tag checks.
Created attachment 340907 [details] [review] client: Port GDaemonFileEnumerator to GTask
Created attachment 340981 [details] [review] common: Port GVfsMountInfo to GTask Added source tag checks...
Review of attachment 340900 [details] [review]: ::: client/gdaemonfile.c @@ +902,3 @@ + g_dbus_error_strip_remote_error (error); + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, + _("Invalid return value from %s"), "query_info"); Why not g_task_return_error? @@ +970,3 @@ data->attributes = g_strdup (attributes); data->flags = flags; data->io_priority = io_priority; We can replace the io_priority member in the struct with g_task_set_priority. @@ +1086,1 @@ data->io_priority = io_priority; Ditto. @@ +1288,3 @@ { + g_dbus_error_strip_remote_error (error); + g_task_return_error (task, error); 'file' is being leaked in this case. @@ -2217,1 @@ - uri = g_file_get_uri (data->file); Nitpick: ideally this should have been in a separate patch, but up to you. @@ +2139,2 @@ data->attributes = g_strdup (attributes); data->io_priority = io_priority; We can replace the io_priority member in the struct with g_task_set_priority. @@ +3150,3 @@ const char *etag, gboolean make_backup, GFileCreateFlags flags, Pre-existing bug: we are losing 'flags'. @@ +3159,3 @@ data->etag = g_strdup (etag ? etag : ""); data->make_backup = make_backup; data->io_priority = io_priority; We can replace the io_priority member in the struct with g_task_set_priority. @@ +3313,3 @@ data->attributes = g_strdup (attributes); data->flags = flags; data->io_priority = io_priority; Ditto. @@ +3462,3 @@ if (! gvfs_dbus_mount_call_set_display_name_finish (proxy, &new_path, res, &error)) { + g_task_return_error (task, error); We are missing a g_dbus_error_strip_remote_error call. @@ +3516,2 @@ data->display_name = g_strdup (display_name); data->io_priority = io_priority; We can replace the io_priority member in the struct with g_task_set_priority. One observation. I see that the io_priority is unused. I wonder if there are places where we implement the asynchronous methods with threads and GSources. If we do, then we can leverage g_task_set_priority to actually use the io_priority. That might require adding a io_priority argument to some internal gvfs methods, though. Hence, not a blocker for this patch.
Thanks for the review! (In reply to Debarshi Ray from comment #77) > Review of attachment 340900 [details] [review] [review]: > > ::: client/gdaemonfile.c > @@ +902,3 @@ > + g_dbus_error_strip_remote_error (error); > + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, > + _("Invalid return value from %s"), > "query_info"); > > Why not g_task_return_error? I don't know, why I used this, probably some copy&paste issue, or rebasing issue... > @@ +970,3 @@ > data->attributes = g_strdup (attributes); > data->flags = flags; > data->io_priority = io_priority; > > We can replace the io_priority member in the struct with g_task_set_priority. Ok > @@ +1086,1 @@ > data->io_priority = io_priority; > > Ditto. > > @@ +1288,3 @@ > { > + g_dbus_error_strip_remote_error (error); > + g_task_return_error (task, error); > > 'file' is being leaked in this case. Good catch, I will fix it... > @@ -2217,1 @@ > - uri = g_file_get_uri (data->file); > > Nitpick: ideally this should have been in a separate patch, but up to you. Ok > @@ +2139,2 @@ > data->attributes = g_strdup (attributes); > data->io_priority = io_priority; > > We can replace the io_priority member in the struct with g_task_set_priority. > > @@ +3150,3 @@ > const char *etag, > gboolean make_backup, > GFileCreateFlags flags, > > Pre-existing bug: we are losing 'flags'. So the daemons never see anything else than G_FILE_CREATE_NONE, good catch! > @@ +3159,3 @@ > data->etag = g_strdup (etag ? etag : ""); > data->make_backup = make_backup; > data->io_priority = io_priority; > > We can replace the io_priority member in the struct with g_task_set_priority. > > @@ +3313,3 @@ > data->attributes = g_strdup (attributes); > data->flags = flags; > data->io_priority = io_priority; > > Ditto. > > @@ +3462,3 @@ > if (! gvfs_dbus_mount_call_set_display_name_finish (proxy, &new_path, > res, &error)) > { > + g_task_return_error (task, error); > > We are missing a g_dbus_error_strip_remote_error call. Ok > @@ +3516,2 @@ > data->display_name = g_strdup (display_name); > data->io_priority = io_priority; > > We can replace the io_priority member in the struct with g_task_set_priority. > > One observation. > > I see that the io_priority is unused. I wonder if there are places where we > implement the asynchronous methods with threads and GSources. If we do, then I don't think so if we can set priority only for GSources (g_thread_set_priority is deprecated)... > we can leverage g_task_set_priority to actually use the io_priority. > > That might require adding a io_priority argument to some internal gvfs > methods, though. Hence, not a blocker for this patch.
Created attachment 341052 [details] [review] gdaemonfile: Remove dead code The uri variable is just set, but never used.
Created attachment 341053 [details] [review] client: Port GDaemonFile to GTask Updated per review...
Created attachment 341054 [details] [review] gdaemonfile: Pass GFileCreateFlags in write operations Flags are not passed in write operations on the client side currently, so the daemons always use G_FILE_CREATE_NONE when writting. Pass the flags in to fix this regression...
Review of attachment 341052 [details] [review]: Thanks for splitting it out. :)
(In reply to Ondrej Holy from comment #78) >> We can replace the io_priority member in the struct with g_task_set_priority. >> >> One observation. >> >> I see that the io_priority is unused. I wonder if there are places where we >> implement the asynchronous methods with threads and GSources. If we do, then > > I don't think so if we can set priority only for GSources > (g_thread_set_priority is deprecated)... Yes, g_thread_set_priority is deprecated, but GTask uses a GThreadPool. The priority set via g_task_set_priority is used to sort the thread pool's internal GAsyncQueue. Grep for g_task_compare_priority. Anyway, it was just a passing thought. Maybe it is not useful after all.
Review of attachment 341054 [details] [review]: ::: client/gdaemonfile.c @@ +3145,3 @@ data->etag = g_strdup (etag ? etag : ""); data->make_backup = make_backup; + data->flags = flags; Nitpick: doing this before the GTask port would let us cherry-pick to stable branches. But you are the maintainer, so I trust you to figure out whatever is best. :)
Review of attachment 341053 [details] [review]: ::: client/gdaemonvfs.c @@ -846,3 @@ /* g_warning ("Error from org.gtk.vfs.MountTracker.lookupMount(): %s", error->message); */ data->callback (NULL, data->user_data, error); - g_error_free (error); We should also remove the g_clear_error in the following else branch. However, I don't really like tying the internals of _g_daemon_vfs_get_mount_info_async to the internals of the caller. I'd prefer to do a g_error_copy in the caller before doing a g_task_return_error. If we do that, then it would also affect commit 811aacef56c2f8ced4f2c8f1e0caf1e819a2eb8e or attachment 338508 [details] [review].
Created attachment 341674 [details] [review] client,common: remove some leftover dead code Porting from GSimpleAsyncResult to GTask resulted in a tiny amount of dead code. Remove that.
Thanks again for another bunch of reviews! (In reply to Debarshi Ray from comment #83) > (In reply to Ondrej Holy from comment #78) > (snip) > > Yes, g_thread_set_priority is deprecated, but GTask uses a GThreadPool. The > priority set via g_task_set_priority is used to sort the thread pool's > internal GAsyncQueue. Grep for g_task_compare_priority. I didn't know, thanks... (In reply to Debarshi Ray from comment #84) > Review of attachment 341054 [details] [review] [review]: > (snip) > > Nitpick: doing this before the GTask port would let us cherry-pick to stable > branches. But you are the maintainer, so I trust you to figure out whatever > is best. :) You are right as always :-) but I don't plan to cherry-pick this to stable, because it seems it was there since ever and I am afraid of the relevant backends codes, because they were probably never been executed... (In reply to Debarshi Ray from comment #85) > Review of attachment 341053 [details] [review] [review]: > > ::: client/gdaemonvfs.c > @@ -846,3 @@ > /* g_warning ("Error from org.gtk.vfs.MountTracker.lookupMount(): > %s", error->message); */ > data->callback (NULL, data->user_data, error); > - g_error_free (error); > > We should also remove the g_clear_error in the following else branch. > > However, I don't really like tying the internals of > _g_daemon_vfs_get_mount_info_async to the internals of the caller. I'd > prefer to do a g_error_copy in the caller before doing a > g_task_return_error. If we do that, then it would also affect commit > 811aacef56c2f8ced4f2c8f1e0caf1e819a2eb8e or attachment 338508 [details] [review] > [review]. Hmm, it is already pretty much tight, but you are right that it would be nicer, will take a look at it...
Attachment 341052 [details] pushed as aa32e74 - gdaemonfile: Remove dead code Attachment 341054 [details] pushed as 41a4f64 - gdaemonfile: Pass GFileCreateFlags in write operations
Review of attachment 341674 [details] [review]: Thanks, looks good! Just please see guidelines for commit messages and improve the message before pushing: https://wiki.gnome.org/Git/CommitMessages - The short description should start with a capital letter... - It can be prefixed with one tag... So, I would use just the following in this case: "Remove some leftover dead code".
Created attachment 341797 [details] [review] gvfsiconloadable: Duplicate caller's error The caller's error is not duplicated since the commit 811aace. It might cause segfaults, because gvfsdaemonvfs is not modified appropriately.
Created attachment 341798 [details] [review] client: Port GDaemonFile to GTask Duplicate the caller's errors also....
Created attachment 341799 [details] [review] client: Do not modify caller's errors Caller's errors are modified by g_dbus_error_strip_remote_error. Strip the dbus error in the caller directly.
Comment on attachment 341797 [details] [review] gvfsiconloadable: Duplicate caller's error Attachment 341797 [details] pushed as d6a678a - gvfsiconloadable: Duplicate caller's error Let's push this before upcomming release in order to avoid possible segfaults...
Pushed commit 7a8cf12 fixing typos in source tag checks, which were causing read failures...
(In reply to Ondrej Holy from comment #94) > Pushed commit 7a8cf12 fixing typos in source tag checks, which were causing > read failures... Oops! Good catch.
Review of attachment 341798 [details] [review]: Thanks for updating the patch, Ondrej. My apologies for not spotting these sooner: ::: client/gdaemonfile.c @@ +620,1 @@ if (connection == NULL) We are leaking 'connection' in the non-NULL case. g_bus_get_finish will return a new ref, which we should drop once we are exiting this function. However, this isn't as bad as it sounds because 'connection' is a singleton. This isn't related to the GTask port. Hence not a blocker for this patch. @@ -3396,3 +3245,2 @@ out: - _g_simple_async_result_complete_with_cancellable (orig_result, data->cancellable); - _g_dbus_async_unsubscribe_cancellable (data->cancellable, data->cancelled_tag); + g_object_unref (task); We shouldn't remove the _g_dbus_async_unsubscribe_cancellable call.
Review of attachment 341799 [details] [review]: ++
Created attachment 344122 [details] [review] client: Port GDaemonFile to GTask Updated per review.
Attachment 341799 [details] pushed as 669f1e5 - client: Do not modify caller's errors Attachment 344122 [details] pushed as c210ccc - client: Port GDaemonFile to GTask
Thanks again for the reviews! (In reply to Debarshi Ray from comment #96) > Review of attachment 341798 [details] [review] [review]: > > Thanks for updating the patch, Ondrej. My apologies for not spotting these > sooner: No problem. > ::: client/gdaemonfile.c > @@ +620,1 @@ > if (connection == NULL) > > We are leaking 'connection' in the non-NULL case. g_bus_get_finish will > return a new ref, which we should drop once we are exiting this function. > However, this isn't as bad as it sounds because 'connection' is a singleton. > > This isn't related to the GTask port. Hence not a blocker for this patch. Will look at it and prepare patch if needed... > @@ -3396,3 +3245,2 @@ > out: > - _g_simple_async_result_complete_with_cancellable (orig_result, > data->cancellable); > - _g_dbus_async_unsubscribe_cancellable (data->cancellable, > data->cancelled_tag); > + g_object_unref (task); > > We shouldn't remove the _g_dbus_async_unsubscribe_cancellable call. Fixed.
Created attachment 344126 [details] [review] udisks2: Port GVfsUDisks2Mount to GTask Rebased on master.
Created attachment 344160 [details] [review] client: Do not leak GDBusConnection reference async_construct_proxy() calls g_object_ref() on connection, because the connection is unrefed by async_call_finish(). The returned connection might be NULL in some cases and g_bus_get() is called instead. However, g_bus_get_finish() is "transfer full" and thus one reference is leaked.
Created attachment 344200 [details] [review] daemon: Port GVfsBackend to GTask Added source tag checks. Fixed some whitespace issues. Fixed broken force unmount ...
Review of attachment 344160 [details] [review]: Looks good to me.
Review of attachment 339050 [details] [review]: Looks good to me.
Thanks for the reviews! Attachment 339050 [details] pushed as 2595478 - client: Port GDaemonFileOutputStream to GTask Attachment 344160 [details] pushed as 1f64137 - client: Do not leak GDBusConnection reference
Review of attachment 339051 [details] [review]: Looks good to me.
Review of attachment 340981 [details] [review]: I am still reviewing this, but I found something: ::: common/gvfsmountinfo.c @@ +533,3 @@ g_free (icon_path); + g_task_return_pointer (task, g_file_icon_new (icon_file), g_object_unref); We are leaking 'icon_file'. The old code was leaking it too, so it is a pre-existing bug.
Review of attachment 340981 [details] [review]: ::: common/gvfsmountinfo.c @@ +520,3 @@ + if (name != NULL) + g_object_set_data_full (G_OBJECT (task), "name", name, g_free); Another pre-existing bug. A few lines above we should be g_strduping the name: - name = meta->di_name; + name = g_strdup (meta->di_name);
Review of attachment 340981 [details] [review]: Apart from the bugs that were already there, this looks good to me. ::: common/gvfsmountinfo.c @@ +161,3 @@ _g_find_file_insensitive_async (root, relative_icon_path, + g_task_get_cancellable (task), I guess it is not worth adding the GCancellable to older branches.
Thanks for the review! Attachment 339051 [details] pushed as 76242c7 - client: Remove obsolete GSimpleAsyncResult helpers Attachment 340981 [details] pushed as 10bf0fa - common: Port GVfsMountInfo to GTask Pushed fixes for the memory issues separately, will backport...
Review of attachment 340907 [details] [review]: ::: client/gdaemonfileenumerator.c @@ +59,3 @@ gulong cancelled_tag; guint timeout_tag; + GTask *task; This member might not be required. See below. @@ +391,1 @@ + g_idle_add ((GSourceFunc ) _g_task_return_pointer_idle_cb, data); We should use the GMainContext that was the thread-default main context at the point where the GTask was created. Instead, this is using the global GMainContext (sometimes referred to as NULL). (This is usually not a problem, unless someone calls g_daemon_file_enumerator_next_files_async in a thread that has a different thread-default main context. That can happen if a synchronous call was implemented in terms of its asynchronous counterpart by wrapping it with a different thread-default context and a GMainLoop. eg., https://git.gnome.org/browse/gnome-online-accounts/tree/src/goabackend/goahttpclient.c#n327) Instead, we need to use g_task_get_context, g_idle_source_new and g_source_attach. g_simple_async_result_complete_in_idle used to do all that for us, but we have to open code it with GTask. @@ +456,3 @@ daemon->async_requested_files = 0; + + g_clear_object (&daemon->task); We might be able to remove this. See below. @@ +602,3 @@ daemon->timeout_tag = 0; daemon->async_requested_files = num_files; + daemon->task = task; Wouldn't it be better to do: g_task_set_task_data (task, g_object_ref (daemon), g_object_unref) ? I am a little worried about the possibility of a reference cycle. 'task' owns a hard reference on 'daemon' through the g_task_new call, and now 'daemon' also has a ref on 'task'. Currently this is fine because the code unref's 'daemon->task' in trigger_async_done, but it can be confusing if the code is moved around later. Also, I think we have been using g_task_set_task_data and passing the GTask around in the other patches, so consistency. :)
Review of attachment 336887 [details] [review]: ::: daemon/gvfsafpconnection.c @@ +886,2 @@ bytes_read = g_input_stream_read_finish (stream, res, &err); if (bytes_read == -1) g_input_stream_read_finish returns a signed gssize, while bytes_read is an unsigned gsize. Hence an equality check against -1 is problematic. I am surprised the compiler isn't warning about this. There are a few more such cases in this file, but this one seems most problematic because it might never detect the error. Might be worth fixing in older branches too. Pre-existing bug, so not a blocker for this patch. @@ -1186,3 @@ const void *buffer; gsize count; int io_priority; We can remove the 'io_priority' member because we are using the GTask to track it. @@ +1193,2 @@ write_data->bytes_written += bytes_written; if (write_data->bytes_written < write_data->count) We are comparing a signed integer with an unsigned one. Might be nice to add a cast. @@ +1196,3 @@ g_output_stream_write_async (stream, (const guint8 *)write_data->buffer + write_data->bytes_written, write_data->count - write_data->bytes_written, We are subtracting a signed integer from an unsigned. A cast might be nice. The compiler usually warns about these, but for some reason it isn't. Probably, we don't have the right flags. Not a blocker for this patch, though. @@ +1197,3 @@ (const guint8 *)write_data->buffer + write_data->bytes_written, write_data->count - write_data->bytes_written, + g_task_get_priority (task), g_task_get_cancellable (task), We are calling g_task_get_priority, but ... @@ -1252,3 @@ write_data->buffer = buffer; write_data->count = count; - write_data->io_priority = io_priority; ... we haven't set it here. @@ +1348,1 @@ while ((req_data = g_queue_pop_head (priv->request_queue))) Aren't we leaking req_data? We are popping it without calling free_request_data. @@ +1348,3 @@ while ((req_data = g_queue_pop_head (priv->request_queue))) { + if (!req_data->task || !g_task_return_error_if_cancelled (req_data->task)) Strictly speaking this new condition isn't the same as the old one. Imagine, req_data->cancellable != NULL g_cancellable_is_cancelled (req_data->cancellable) == TRUE req_data->simple == NULL In that case we wouldn't have hit the 'break', but now we will. @@ +1465,1 @@ req_data->conn = afp_connection; We can drop req_data->conn because req_data->task already has it. Not a big deal, though. Up to you. @@ +1485,3 @@ GError **error) { + g_return_val_if_fail (g_task_is_valid (res, afp_connection), NULL); Maybe you want to check the source tag too? @@ -1738,3 @@ -#define REQUEST_DATA_CLOSED(request_data) { \ - g_simple_async_result_set_from_error (req_data->simple, \ The old code had a typo. request_data versus req_data. :)
*** Bug 782195 has been marked as a duplicate of this bug. ***
(In reply to Debarshi Ray from comment #112) > Review of attachment 340907 [details] [review] [review]: Thanks for the new reviews! I really keen on consistency, but I made an exception here, because the sync and async codes are pretty interconnected and I don't want to touch the logic, but I will try to split it somehow...
Please skip the AFP patches now and let's review them as the last because I don't have an environment where I can test them (I will try to configure netatalk later)... The common parts are more important than the backend anyway...
Created attachment 352770 [details] [review] client: Port GDaemonFileEnumerator to GTask Source tag checks added. Fixed _g_task_return_pointer_idle. "changed" signal is added in order to allow splitting _sync and _async code paths to use GTask as user_data for _async functionality.
Created attachment 352771 [details] [review] udisks2: Port GVfsUDisks2Drive to GTask Source tag checks added.
Created attachment 352772 [details] [review] udisks2: Port GVfsUDisks2Volume to GTask Source tag checks added.
Created attachment 352773 [details] [review] udisks2: Port GVfsUDisks2Utils to GTask Source tag checks added.
Created attachment 352774 [details] [review] udisks2: Port GVfsUDisks2Mount to GTask Source tag checks added.
Created attachment 352775 [details] [review] proxy: Port GProxyVolume to GTask Source tag checks added.
Created attachment 352776 [details] [review] proxy: Port GProxyMount to GTask Source tag checks added.
Created attachment 352777 [details] [review] proxy: Port GProxyDrive to GTask Source tag checks added.
Review of attachment 352770 [details] [review]: Two minor things. Feel free to commit after considering them. ::: client/gdaemonfileenumerator.c @@ -191,3 @@ enumerator->done = TRUE; - if (enumerator->async_requested_files > 0) - trigger_async_done (enumerator, TRUE); This used to run with the 'infos' lock held. @@ +207,3 @@ G_UNLOCK (infos); + g_signal_emit (enumerator, signals[CHANGED], 0); Now it runs after an unlock/lock. Is that a problem? In theory, things can change between the unlock/lock. The purpose of the lock isn't clear to me, so I don't know if that matters or not. @@ -233,3 @@ - if (enumerator->async_requested_files > 0 && - g_list_length (enumerator->infos) >= enumerator->async_requested_files) - trigger_async_done (enumerator, TRUE); Same here. @@ +602,3 @@ + + task = g_task_new (enumerator, cancellable, callback, user_data); + g_task_set_source_tag (task, g_daemon_file_enumerator_next_files_async); Maybe we should set the io_priority on the GTask? We are using it now.
(In reply to Debarshi Ray from comment #125) > Review of attachment 352770 [details] [review] [review]: > > Two minor things. Feel free to commit after considering them. > > ::: client/gdaemonfileenumerator.c > @@ -191,3 @@ > enumerator->done = TRUE; > - if (enumerator->async_requested_files > 0) > - trigger_async_done (enumerator, TRUE); > > This used to run with the 'infos' lock held. > > @@ +207,3 @@ > G_UNLOCK (infos); > > + g_signal_emit (enumerator, signals[CHANGED], 0); > > Now it runs after an unlock/lock. Is that a problem? In theory, things can > change between the unlock/lock. The purpose of the lock isn't clear to me, > so I don't know if that matters or not. I suppose that the lock is here to handle cases when you run g_file_enumerator_next_file(_async) from different threads concurrently, although I think it doesn't make much sense in ordinary cases. The file order is not guaranteed anyway, so I think it doesn't matter...
(In reply to Ondrej Holy from comment #126) > (In reply to Debarshi Ray from comment #125) > > Now it runs after an unlock/lock. Is that a problem? In theory, things can > > change between the unlock/lock. The purpose of the lock isn't clear to me, > > so I don't know if that matters or not. > > I suppose that the lock is here to handle cases when you run > g_file_enumerator_next_file(_async) from different threads concurrently, > although I think it doesn't make much sense in ordinary cases. The file > order is not guaranteed anyway, so I think it doesn't matter... Ok. Changing the status of attachment 352770 [details] [review]
Created attachment 353740 [details] [review] client: Port GDaemonFileEnumerator to GTask Added g_task_set_priority...
Comment on attachment 353740 [details] [review] client: Port GDaemonFileEnumerator to GTask Attachment 353740 [details] pushed as 2eae108 - client: Port GDaemonFileEnumerator to GTask
(In reply to Debarshi Ray from comment #112) > Instead, we need to use g_task_get_context, g_idle_source_new and > g_source_attach. g_simple_async_result_complete_in_idle used to do all that > for us, but we have to open code it with GTask. I forgot that g_task_attach_source exists. :(
Created attachment 353824 [details] [review] gdaemonfileenumerator: Marginally simplify the code This will pollute the history, so feel free to ignore it.
(In reply to Debarshi Ray from comment #113) > Review of attachment 336887 [details] [review] [review]: > > ::: daemon/gvfsafpconnection.c > @@ +886,2 @@ > bytes_read = g_input_stream_read_finish (stream, res, &err); > if (bytes_read == -1) > > g_input_stream_read_finish returns a signed gssize, while bytes_read is an > unsigned gsize. Hence an equality check against -1 is problematic. I am > surprised the compiler isn't warning about this. This may work correctly, but probably depends on compiler and compiler flags... but will fix that. > There are a few more such cases in this file, but this one seems most > problematic because it might never detect the error. Might be worth fixing > in older branches too. > > Pre-existing bug, so not a blocker for this patch. > > @@ -1186,3 @@ > const void *buffer; > gsize count; > int io_priority; > > We can remove the 'io_priority' member because we are using the GTask to > track it. > > @@ +1193,2 @@ > write_data->bytes_written += bytes_written; > if (write_data->bytes_written < write_data->count) > > We are comparing a signed integer with an unsigned one. Might be nice to add > a cast. > > @@ +1196,3 @@ > g_output_stream_write_async (stream, > (const guint8 *)write_data->buffer + > write_data->bytes_written, > write_data->count - > write_data->bytes_written, > > We are subtracting a signed integer from an unsigned. A cast might be nice. > > The compiler usually warns about these, but for some reason it isn't. > Probably, we don't have the right flags. Not a blocker for this patch, > though. > > @@ +1197,3 @@ > (const guint8 *)write_data->buffer + > write_data->bytes_written, > write_data->count - > write_data->bytes_written, > + g_task_get_priority (task), > g_task_get_cancellable (task), > > We are calling g_task_get_priority, but ... > > @@ -1252,3 @@ > write_data->buffer = buffer; > write_data->count = count; > - write_data->io_priority = io_priority; > > ... we haven't set it here. > > @@ +1348,1 @@ > while ((req_data = g_queue_pop_head (priv->request_queue))) > > Aren't we leaking req_data? We are popping it without calling > free_request_data. The RequestData memory management looks a bit weird, because the RequestData are stored in request_hash and freed on various places... I will try to fix this as a part of Bug 783856. > @@ +1348,3 @@ > while ((req_data = g_queue_pop_head (priv->request_queue))) > { > + if (!req_data->task || !g_task_return_error_if_cancelled > (req_data->task)) > > Strictly speaking this new condition isn't the same as the old one. Imagine, > req_data->cancellable != NULL > g_cancellable_is_cancelled (req_data->cancellable) == TRUE > req_data->simple == NULL > > In that case we wouldn't have hit the 'break', but now we will. I am convinced that this is ok, because this situation can't happen (cancellable can't exists without task). > @@ +1465,1 @@ > req_data->conn = afp_connection; > > We can drop req_data->conn because req_data->task already has it. Not a big > deal, though. Up to you. I don't think so, because task is not created in some cases, but conn is always specified... > @@ +1485,3 @@ > GError **error) > { > + g_return_val_if_fail (g_task_is_valid (res, afp_connection), NULL); > > Maybe you want to check the source tag too? I will add source tag checks in the afp patches and fix other things you mentioned here...
Created attachment 353873 [details] [review] afp: Prevent comparsion between signed and unsigned Swap gssize and gsize data types in order to prevent comparisons and other operations between signed and unsigned types, which might lead to troubles. I think that this is better than casts and similar what is already done in other cases...
Created attachment 353874 [details] [review] afp: Port GVfsBackendAfpBrowse to GTask Added source tag checks...
Created attachment 353875 [details] [review] afp: Port GVfsAfpServer to GTask Added source tag checks...
Created attachment 353876 [details] [review] afp: Port GVfsAfpConnection to GTask Added source tag checks and fix priority handling...
Created attachment 353877 [details] [review] afp: Port GVfsAfpVolume to GTask Added source tag checks...
Review of attachment 353824 [details] [review]: Thanks. I usually don't like such code changes, but it makes sense and can be useful later when I will copy&paste this code into other places :-D Just the commit description might be rather something like "Use g_task_attach_source instead g_source_attach" or so (I've thought initially that you rewrote the whole enumerator code, which would be also useful :-D)...
Comment on attachment 353740 [details] [review] client: Port GDaemonFileEnumerator to GTask This patch maybe causes Bug 784953...
Comment on attachment 353824 [details] [review] gdaemonfileenumerator: Marginally simplify the code Attachment 353824 [details] pushed as 9274d41 - gdaemonfileenumerator: Marginally simplify the code
I have been doing another round of testing with attached patches even on afp shares and seemed working correctly. Let's push the remaining patches now. Nobody has time for reviews, but it is still time for eventual fixes before stable release...
Attachment 344200 [details] pushed as e28e7c2 - daemon: Port GVfsBackend to GTask Attachment 352771 [details] pushed as ff0a2d2 - udisks2: Port GVfsUDisks2Drive to GTask Attachment 352772 [details] pushed as 17e630d - udisks2: Port GVfsUDisks2Volume to GTask Attachment 352773 [details] pushed as 1f31d2c - udisks2: Port GVfsUDisks2Utils to GTask Attachment 352774 [details] pushed as 83bde0c - udisks2: Port GVfsUDisks2Mount to GTask Attachment 352775 [details] pushed as 1f5fcbe - proxy: Port GProxyVolume to GTask Attachment 352776 [details] pushed as 42e740c - proxy: Port GProxyMount to GTask Attachment 352777 [details] pushed as 5af38d0 - proxy: Port GProxyDrive to GTask Attachment 353873 [details] pushed as 7e663df - afp: Prevent comparsion between signed and unsigned Attachment 353874 [details] pushed as 50a95d6 - afp: Port GVfsBackendAfpBrowse to GTask Attachment 353875 [details] pushed as 5dc1a3e - afp: Port GVfsAfpServer to GTask Attachment 353876 [details] pushed as 43ed797 - afp: Port GVfsAfpConnection to GTask Attachment 353877 [details] pushed as 48d0385 - afp: Port GVfsAfpVolume to GTask