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 700756 - GFile.new_for_path arguments misses (type filename) annotation
GFile.new_for_path arguments misses (type filename) annotation
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-05-21 00:03 UTC by lamefun
Modified: 2016-06-15 17:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (9.33 KB, patch)
2013-05-24 04:59 UTC, lamefun
none Details | Review
gfileutils: Fix a signed/unsigned integer comparison (1.03 KB, patch)
2014-11-27 09:19 UTC, Philip Withnall
committed Details | Review
gio: Add missing (type filename) annotations (4.38 KB, patch)
2014-11-27 09:20 UTC, Philip Withnall
none Details | Review
gfileutils: Add missing (type filename) annotations (9.24 KB, patch)
2014-11-27 09:20 UTC, Philip Withnall
none Details | Review
gio: Add missing (type filename) annotations (1.12 KB, patch)
2016-06-15 15:07 UTC, Philip Withnall
committed Details | Review
gfileutils: Add missing (type filename) annotations (3.68 KB, patch)
2016-06-15 15:07 UTC, Philip Withnall
committed Details | Review

Comment 1 lamefun 2013-05-21 00:04:39 UTC
And so seem g_file_get_path, g_file_get_name, etc.
Comment 2 Matthias Clasen 2013-05-24 02:40:32 UTC
A patch would be welcome for this
Comment 3 lamefun 2013-05-24 04:59:48 UTC
Created attachment 245210 [details] [review]
Patch

I found some of the most obvious ones, I still have doubts about g_build_path. Is it intended as a general-purpose function for all types of strings?
Comment 4 Dan Winship 2013-05-24 13:26:19 UTC
(In reply to comment #3)
> I still have doubts about g_build_path.
> Is it intended as a general-purpose function for all types of strings?

It's used for a non-filename in gutils.c.

(If g_build_filename() gets annotated and g_build_path() does not, then the g_build_filename() docs need to be updated to not claim they behave the same way on UNIX.)

g_path_is_absolute() is probably also used for non-filenames in places, but that can safely be just left unannotated, since it will still give the same result if you have to convert your filesystem-charset path into a UTF-8 path before calling it.
Comment 5 Philip Withnall 2014-11-27 09:19:56 UTC
Created attachment 291622 [details] [review]
gfileutils: Fix a signed/unsigned integer comparison

Also use size_t rather than int, allowing for larger files to be
handled.
Comment 6 Philip Withnall 2014-11-27 09:20:00 UTC
Created attachment 291623 [details] [review]
gio: Add missing (type filename) annotations

These differentiate between strings in the GLib filename encoding, and
strings in UTF-8.
Comment 7 Philip Withnall 2014-11-27 09:20:04 UTC
Created attachment 291624 [details] [review]
gfileutils: Add missing (type filename) annotations

These differentiate between strings in the GLib filename encoding, and
strings in UTF-8.
Comment 8 Philip Withnall 2014-11-27 09:22:44 UTC
Comment on attachment 245210 [details] [review]
Patch

Updated patches. Mostly the same; I added a few missing annotations to gfileutils.c, removed them from g_file_new_for_commandline_arg[_and_cwd]() since they explicitly take UTF-8 args, but added them to g_application_command_line_get_cwd().

I think g_build_path() and g_build_basename() should both have the annotations. Any case where the functions are (I guess) being abused for non-filenames can always cast (or equivalent in the binding language) to avoid encoding conversion.
Comment 9 Christoph Reiter (lazka) 2016-06-04 19:29:32 UTC
I've just pushed two patches in bug 767245 to add filename annotations which also covers these cases, so the annotation patches should be obsolete.

The g_file_read_link still applies. But, as far as I can see int will be converted to uint in the comparison and when uint at the end of the loop overflows it will just start reading smaller chunks again, so while a bug, it still produces a valid result.
Comment 10 Philip Withnall 2016-06-15 15:00:40 UTC
Comment on attachment 291622 [details] [review]
gfileutils: Fix a signed/unsigned integer comparison

Pushing this one in response to the almost-review. It's fairly trivial.

Attachment 291622 [details] pushed as c914114 - gfileutils: Fix a signed/unsigned integer comparison
Comment 11 Philip Withnall 2016-06-15 15:07:00 UTC
(In reply to Christoph Reiter (lazka) from comment #9)
> I've just pushed two patches in bug 767245 to add filename annotations which
> also covers these cases, so the annotation patches should be obsolete.
> 
> The g_file_read_link still applies. But, as far as I can see int will be
> converted to uint in the comparison and when uint at the end of the loop
> overflows it will just start reading smaller chunks again, so while a bug,
> it still produces a valid result.

The patch to gfileutils still applies. There are two remaining additions in the other patch, which I still think are valid:
 - g_file_get_child()
 - g_file_resolve_relative_path()

Updated patches coming, rebased on master.
Comment 12 Philip Withnall 2016-06-15 15:07:11 UTC
Created attachment 329862 [details] [review]
gio: Add missing (type filename) annotations

These differentiate between strings in the GLib filename encoding, and
strings in UTF-8.
Comment 13 Philip Withnall 2016-06-15 15:07:15 UTC
Created attachment 329863 [details] [review]
gfileutils: Add missing (type filename) annotations

These differentiate between strings in the GLib filename encoding, and
strings in UTF-8.
Comment 14 Christoph Reiter (lazka) 2016-06-15 16:09:41 UTC
Review of attachment 329862 [details] [review]:

lgtm
Comment 15 Christoph Reiter (lazka) 2016-06-15 16:11:21 UTC
Review of attachment 329863 [details] [review]:

lgtm
Comment 16 Philip Withnall 2016-06-15 17:24:43 UTC
Thanks.

Attachment 329862 [details] pushed as ac11666 - gio: Add missing (type filename) annotations
Attachment 329863 [details] pushed as ec40e9d - gfileutils: Add missing (type filename) annotations