GNOME Bugzilla – Bug 767976
GFile: Add g_file_peek_path()
Last modified: 2018-01-15 18:27:59 UTC
I've been carrying `gs_file_get_path_cached()` in libgsystem and it has seen a lot of use in the ostree and flatpak codebases. There are probably others too. I think language bindings like Python/Gjs could also use this to avoid an extra malloc (i.e. we could transparently replace `g_file_get_path()` with `g_file_get_cpath()`.
Created attachment 330252 [details] [review] GFile: Add g_file_get_cpath()
Note - I'm not totally sure about the value of this, because at least for OSTree I'm likely to rework things to avoid GFile internally for the most part. I'm curious what Alex (flatpak) thinks though.
I realise that since this was filed, libostree has moved away from GFile — and that’s totally appropriate for libostree and its level in the stack. However, I think there’s definitely value in a g_file_get_cpath() method for everywhere higher up in the stack — it’s always annoyed me that g_file_get_path() does an extra malloc() and you have to worry about freeing it. I’m not totally sold on the naming (people might ask ‘what is a cpath?’). Maybe g_file_peek_path() instead? Open to other suggestions. Matthias, Alex, what do you think about this? I’d like to get it in (or resolved, in any case) for 2.56.
(In reply to Philip Withnall from comment #3) > Matthias, Alex, what do you think about this? I’d like to get it in (or > resolved, in any case) for 2.56. Ping?
I hate to add a name with a easily overlooked one-character different to the existing g_file_get_path Can we be explicit and call it g_file_get_cached_path() ?
(In reply to Matthias Clasen from comment #5) > Can we be explicit and call it g_file_get_cached_path() ? I’d marginally prefer g_file_peek_path() because it’s less typing (and this will be a commonly used function), but I definitely agree that get_cpath() is to be avoided.
I agree with g_file_peek_path()
Review of attachment 330252 [details] [review]: ::: gio/gfile.c @@ +576,3 @@ + _file_path_quark = g_quark_from_static_string ("gio-file-path"); + + G_LOCK (pathname_cache); Why is locking needed here? @@ +615,3 @@ + */ +const char * +g_file_get_cpath (GFile *file) Rename to g_file_peek_path().
Created attachment 366545 [details] [review] GFile: Add g_file_peek_path() This is a variant of g_file_get_path() which returns a const string to the caller, rather than transferring ownership. I've been carrying `gs_file_get_path_cached()` in libgsystem and it has seen a lot of use in the ostree and flatpak codebases. There are probably others too. I think language bindings like Python/Gjs could also use this to avoid an extra malloc (i.e. we could transparently replace `g_file_get_path()` with `g_file_peek_path()`. (Originally by Colin Walters. Tweaked by Philip Withnall to update to 2.56 and change the function name.)
OK, here’s an updated version of the patch with my review comments (except the query about the need for locking) fixed, plus a few other trivial cleanups made. Can someone else give this a review please? Colin, what’s the deal with the locking?
I suppose the API could not be threadsafe but I think you'd find a lot of people expect "const accessors" i.e. _get() functions to be threadsafe. At least in libostree and rpm-ostree we end up passing GFiles into worker threads. Or at least we used to - of course we're pretty fd-relative nowadays.
I’m not quite sure I follow. What specific situations do you expect this locking to help in?
https://git.gnome.org/browse/libgsystem/commit/?id=2d24676846e3058d7a03bc127591f9fdcc346fab
Created attachment 366649 [details] [review] GFile: Add g_file_peek_path() This is a variant of g_file_get_path() which returns a const string to the caller, rather than transferring ownership. I've been carrying `gs_file_get_path_cached()` in libgsystem and it has seen a lot of use in the ostree and flatpak codebases. There are probably others too. I think language bindings like Python/Gjs could also use this to avoid an extra malloc (i.e. we could transparently replace `g_file_get_path()` with `g_file_peek_path()`. (Originally by Colin Walters. Tweaked by Philip Withnall to update to 2.56 and change the function name.)
(In reply to Colin Walters from comment #13) > https://git.gnome.org/browse/libgsystem/commit/ > ?id=2d24676846e3058d7a03bc127591f9fdcc346fab That explains things well, thanks. I’ve added a similar explanation in a comment. Patch ready for review again.
Review of attachment 366649 [details] [review]: ::: gio/gfile.c @@ +598,3 @@ + return NULL; + } + g_object_set_qdata_full ((GObject*)file, _file_path_quark, (char*)path, (GDestroyNotify)g_free); This works, but I wonder if we really need a separate lock here - g_object_set_data _is_ threadsafe, and we have an atomic replace operation for it: g_object_replace_data which seems like it would be a perfect fit here.
Review of attachment 366649 [details] [review]: ::: gio/gfile.c @@ +598,3 @@ + return NULL; + } + g_object_set_qdata_full ((GObject*)file, _file_path_quark, (char*)path, (GDestroyNotify)g_free); Ah, yes, g_object_replace_data() would work well, thanks.
We don't want to replace the original data; a thread could be using it. We want something like `g_object_set_data_if_not_exists`.
(In reply to Colin Walters from comment #18) > We don't want to replace the original data; a thread could be using it. We > want something like `g_object_set_data_if_not_exists`. I spent 5 minutes thinking that, but g_object_replace_data() is actually a standard compare-and-swap: it only does the replacement if the old data in the GObject matches the old_data parameter passed in to g_object_replace_data().
Created attachment 366840 [details] [review] GFile: Add g_file_peek_path() This is a variant of g_file_get_path() which returns a const string to the caller, rather than transferring ownership. I've been carrying `gs_file_get_path_cached()` in libgsystem and it has seen a lot of use in the ostree and flatpak codebases. There are probably others too. I think language bindings like Python/Gjs could also use this to avoid an extra malloc (i.e. we could transparently replace `g_file_get_path()` with `g_file_peek_path()`. (Originally by Colin Walters. Tweaked by Philip Withnall to update to 2.56, change the function name and drop the locking.)
(In reply to Philip Withnall from comment #20) > Created attachment 366840 [details] [review] [review] > GFile: Add g_file_peek_path() Updated to use g_object_replace_qdata(). I’m currently testing it.
Created attachment 366841 [details] [review] GFile: Add g_file_peek_path() This is a variant of g_file_get_path() which returns a const string to the caller, rather than transferring ownership. I've been carrying `gs_file_get_path_cached()` in libgsystem and it has seen a lot of use in the ostree and flatpak codebases. There are probably others too. I think language bindings like Python/Gjs could also use this to avoid an extra malloc (i.e. we could transparently replace `g_file_get_path()` with `g_file_peek_path()`. (Originally by Colin Walters. Tweaked by Philip Withnall to update to 2.56, change the function name and drop the locking.)
Updated to fix a compiler warning. Tested, and it works.
Review of attachment 366841 [details] [review]: ::: gio/gfile.c @@ +599,3 @@ + + if (g_object_replace_qdata ((GObject *) file, _file_path_quark, + NULL, (gpointer) path, OK I wouldn't have guessted that `_replace_qdata()` had those semantics. Hmm so I had to think about this hard, but I think it's correct... basically since we're passing NULL here for `olddata`, it will never match, and hence we'll never replace existing data. This is making me wonder though...what other software is relying on those semantics? *Nothing* in libglib today besides test cases uses either API. In a subset of git.gnome.org checkouts I have locally: $ for x in *; do (cd $x && git grep g_object_replace_qdata | cat); done pango/pango-context.c: if (!g_object_replace_qdata (G_OBJECT (fontset), cache_quark, NULL, pango/pangocairo-context.c: if (!g_object_replace_qdata (G_OBJECT (context), context_info_quark, NULL, $ Which...indeed looks logically the same, including passing NULL for `olddata`.
Review of attachment 366841 [details] [review]: Marking ACN from my side, though an optional bit would be to add a comment like: /* By passing NULL here we ensure we never replace existing data */ or so?
Review of attachment 366841 [details] [review]: ::: gio/gfile.c @@ +599,3 @@ + + if (g_object_replace_qdata ((GObject *) file, _file_path_quark, + NULL, (gpointer) path, Thanks for that analysis; looking at the Pango code reminded me I haven’t free the new path in the failure case of g_object_replace_qdata(). I’ll fix that before pushing.
Attachment 366841 [details] pushed as 4808a95 - GFile: Add g_file_peek_path()