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 655470 - gvfs-open exits with error code 0 (success) even when opening fails (file not found or default app not registered)
gvfs-open exits with error code 0 (success) even when opening fails (file not...
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2011-07-28 04:07 UTC by Rodrigo Silva
Modified: 2011-08-24 10:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch gvfs-open so exit code = 1 when it fails to open any file (2.66 KB, patch)
2011-07-28 06:23 UTC, Rodrigo Silva
none Details | Review
Updated patch, with cosmetic changes (spacing, line breaks, commit message) (2.66 KB, patch)
2011-07-28 08:44 UTC, Rodrigo Silva
committed Details | Review
Improved (and corrected) commit message. Minor cosmetic change to code (1.40 KB, patch)
2011-08-24 04:03 UTC, Rodrigo Silva
none Details | Review

Description Rodrigo Silva 2011-07-28 04:07:31 UTC
Not sure if this is by design, but gvfs-open exits with success even when operation fails

The following conditions should exit with error code > 0:
- File is not found
- File is found, but default application for opening it is not registered

Examples:
$ gvfs-open nofile.txt || echo "NOT FOUND!"
gvfs-open: file:///home/rodrigo/nofile.txt: error opening location: Error stating file '/home/rodrigo/nofile.txt': No such file or directory

$ touch test.mdb ; gvfs-open nofile.txt || echo "NO ASSOCIATION!" ; rm test.mdb
gvfs-open: file:///home/rodrigo/nofile.txt: error opening location: Error stating file '/home/rodrigo/nofile.txt': No such file or directory

In both situations exit code was 0, meaning success, even when gvfs-open was clearly NOT successful. This makes gvfs-open hard to use in scripts.

And the problem is even worse considering gvfs-open is used by xdg-open

Using Maverick 10.10 64bits, gvfs 1.6.4-0ubuntu1.1

I already have fixed the source code, and would gladly provide a patch if anyone could tell me how to compile the source from master git, since it requires glib >= 2.27.4 and I currently only have glib 2.26.1
Comment 1 Rodrigo Silva 2011-07-28 06:23:57 UTC
Created attachment 192795 [details] [review]
Patch gvfs-open so exit code = 1 when it fails to open any file

Compiled and tested in 1.6.1 branch. Should work in latest branch since this file is not changes since 2009
Comment 2 Rodrigo Silva 2011-07-28 08:44:38 UTC
Created attachment 192800 [details] [review]
Updated patch, with cosmetic changes (spacing, line breaks, commit message)
Comment 3 Tomas Bzatek 2011-08-23 14:48:25 UTC
Thanks for the patch, I've committed slightly modified one, based on your ideas. 

commit b76c3e8053a44b971c5fc8b6ff9d5adfb6557eb5
Author: Rodrigo Silva <gnome@rodrigosilva.com>
Date:   Tue Aug 23 16:47:09 2011 +0200

    gvfs-open: Exit with error code > 0 when open fails
    
    Previously gvfs-open exited with success (error code 0) even when open
    failed, either because file was not found or there was no default
    application registred to open it. Without easy testing for error codes,
    it was hard to use it in scripts. Now it exits with code 1 when any of
    the given files are either not found or its default application is not
    registered.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=655470
Comment 4 Rodrigo Silva 2011-08-23 17:17:56 UTC
Hi Tomas!

Im glad (and flattered) my patch made into Gnome. This is my 1st code contribution to free software, and im honored to be among giants, and see my name listed as Author in git's commit log.

Some questions:

- Is there any way this patch can be cherry-picked to gnome-2-32 branch? That is the branch followed by Ubuntu Maverick, so being there is the only hope I have that i will someday see my own fix via apt-get in Maverick updates repo.

- Shouldn't commit message be updated to "Now it exits with code 2" instead of "code 1" to reflect the change you made in my patch?
Comment 5 Rodrigo Silva 2011-08-24 04:03:54 UTC
Created attachment 194544 [details] [review]
Improved (and corrected) commit message. Minor cosmetic change to code

Tomas, given your modifications to my patch were so elegant and improved, i took the liberty to make a minor change: res is now gboolean instead of int, to avoid the implicit type conversion and "boolean hack" in res += !(open). Granted, internally gboolean is typedef'ed to int... but, in a purist point of view, since open returns gboolean, res = open && res describes more clearly what we are tying to do. (i just wished C had &&= attribution.. the logical one, not the bitwise)

Anyway, feel free to discard the patch code if such cosmetic change is not enough to warrant a new commit. But i still think the commit message should be at least amended: i fixed some typos, corrected the "exit with code 1" to "code 2", removed that nasty TAB char in title, and did other minor improvements in description.
Comment 6 Tomas Bzatek 2011-08-24 10:27:48 UTC
(In reply to comment #4)
> - Is there any way this patch can be cherry-picked to gnome-2-32 branch? That
> is the branch followed by Ubuntu Maverick, so being there is the only hope I
> have that i will someday see my own fix via apt-get in Maverick updates repo.
I don't plan to make any more releases in gnome-2-32 branch, distributors are free to pick any patch naturally. Not sure if there's going to be a release in gnome-3-0 branch even.

> - Shouldn't commit message be updated to "Now it exits with code 2" instead of
> "code 1" to reflect the change you made in my patch?

Ah yeah, overlooked that. It's too late to change commit message anyway, git can't do that.

(In reply to comment #5)
> Tomas, given your modifications to my patch were so elegant and improved, i
> took the liberty to make a minor change: res is now gboolean instead of int, to
> avoid the implicit type conversion and "boolean hack" in res += !(open).
> Granted, internally gboolean is typedef'ed to int... but, in a purist point of
> view, since open returns gboolean, res = open && res describes more clearly
> what we are tying to do. (i just wished C had &&= attribution.. the logical
> one, not the bitwise)
> 
> Anyway, feel free to discard the patch code if such cosmetic change is not
> enough to warrant a new commit. But i still think the commit message should be
> at least amended: i fixed some typos, corrected the "exit with code 1" to "code
> 2", removed that nasty TAB char in title, and did other minor improvements in
> description.

I appreciate your effort, but I'm scared of the shortened boolean eval even if it should be working - I originally had a gboolean definition just like you but changed to int, it's more safe. I don't trust modern compilers, seen too many probles with miscompilations and we still need to support ancient compilers. I'd rather stay with the current code, we don't need to be 100% clean.