After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 739075 - GDaemonFile doesn't always set an error if we don't have a mount
GDaemonFile doesn't always set an error if we don't have a mount
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: general
git master
Other All
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2014-10-23 14:20 UTC by Debarshi Ray
Modified: 2014-11-04 21:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
client: Don't guard against g_object_new returning NULL (1.65 KB, patch)
2014-10-23 14:32 UTC, Debarshi Ray
none Details | Review
client: Don't guard against g_object_new returning NULL (1.66 KB, patch)
2014-10-23 14:34 UTC, Debarshi Ray
committed Details | Review
client: Always set an error if we don't have a mount (1.00 KB, patch)
2014-10-23 14:34 UTC, Debarshi Ray
committed Details | Review
client: Remove inconsistencies between async and sync variants (1.21 KB, patch)
2014-10-23 14:34 UTC, Debarshi Ray
committed Details | Review
client: Fix up error handling in find_enclosing_mount (1.65 KB, patch)
2014-11-03 22:54 UTC, Ross Lagerwall
committed Details | Review

Description Debarshi Ray 2014-10-23 14:20:24 UTC
I was playing with my WIP Google Drive backend, when I noticed that GDaemonFile does not always set an error if find_enclosing_mount_async failed to get a mount.
Comment 1 Debarshi Ray 2014-10-23 14:32:32 UTC
Created attachment 289206 [details] [review]
client: Don't guard against g_object_new returning NULL
Comment 2 Debarshi Ray 2014-10-23 14:34:06 UTC
Created attachment 289207 [details] [review]
client: Don't guard against g_object_new returning NULL

Fix a typo in the commit message.
Comment 3 Debarshi Ray 2014-10-23 14:34:30 UTC
Created attachment 289208 [details] [review]
client: Always set an error if we don't have a mount
Comment 4 Debarshi Ray 2014-10-23 14:34:56 UTC
Created attachment 289209 [details] [review]
client: Remove inconsistencies between async and sync variants
Comment 5 Ross Lagerwall 2014-10-23 20:39:38 UTC
Review of attachment 289207 [details] [review]:

Looks good.
Comment 6 Ondrej Holy 2014-10-24 15:10:23 UTC
Review of attachment 289208 [details] [review]:

Looks good, thanks.
Comment 7 Ondrej Holy 2014-10-24 15:10:42 UTC
Review of attachment 289209 [details] [review]:

Not sure whether this situation could happen, however it is just removing inconsistencies, and we have to set error when returning NULL (according the gio doc).
Comment 8 Debarshi Ray 2014-10-29 12:45:50 UTC
Thanks for the reviews.
Comment 9 Ross Lagerwall 2014-11-03 22:54:03 UTC
Created attachment 289952 [details] [review]
client: Fix up error handling in find_enclosing_mount

2a0dc68c7eeb ("client: Remove inconsistencies between async and sync
variants") introduced a regression which causes find_enclosing_mount to
segfault if a non-NULL GError** is passed in.  Fix this.  Also, only
call g_dbus_error_strip_remote_error () on GErrors that have come from
dbus.
Comment 10 Ross Lagerwall 2014-11-03 22:54:44 UTC
Reopening due to some breakage in the last commit...
Comment 11 Debarshi Ray 2014-11-04 12:00:17 UTC
Review of attachment 289952 [details] [review]:

I am not a gvfs maintainer so I can neither ACK nor NACK, but I took a look since it was me who caused the regression. Thanks for cleaning up after me, Ross.

::: client/gdaemonfile.c
@@ +2280,3 @@
 						  error);
 
+  if (error && *error)

Oops! This is obviously correct.

@@ -2313,3 @@
-out:
-  if (error && *error)
-    g_dbus_error_strip_remote_error (*error);

Since g_dbus_error_strip_remote_error does not complain if the error did not come over D-Bus, this is a relatively cosmetic change.
Comment 12 Ondrej Holy 2014-11-04 14:05:56 UTC
Review of attachment 289952 [details] [review]:

Ross, thanks for that!
Comment 13 Ross Lagerwall 2014-11-04 21:55:33 UTC
(In reply to comment #11)
> Review of attachment 289952 [details] [review]:
> 
> I am not a gvfs maintainer so I can neither ACK nor NACK, but I took a look
> since it was me who caused the regression. Thanks for cleaning up after me,
> Ross.
> 
> ::: client/gdaemonfile.c
> @@ +2280,3 @@
>                            error);
> 
> +  if (error && *error)
> 
> Oops! This is obviously correct.
> 
> @@ -2313,3 @@
> -out:
> -  if (error && *error)
> -    g_dbus_error_strip_remote_error (*error);
> 
> Since g_dbus_error_strip_remote_error does not complain if the error did not
> come over D-Bus, this is a relatively cosmetic change.

Yeah, I realise, but it seemed cleaner like this after moving the error check up.
Comment 14 Ross Lagerwall 2014-11-04 21:55:44 UTC
Pushed to master as 7757c2bcfef808c860ec0e8676d69301a0f821d1. Thanks for the review.