GNOME Bugzilla – Bug 659754
Add API to GMappedFile that allows to pass FD, so that O_NOATIME can be used
Last modified: 2011-09-21 23:03:38 UTC
Created attachment 197179 [details] [review] Patch that implements this proposal At Tracker we want to open files using O_NOATIME. With GMappedFile this is currently impossible. For that reason I propose to add the API g_mapped_file_new_from_fd. I have attached a patch that implements such an API. I didn't let g_mapped_file_new reuse g_mapped_file_new_from_fd because otherwise the error messages couldn't use the display_filename.
Example of a use-case where we had to stop using GMappedFile and start using mmap manually (unfortunately): http://git.gnome.org/browse/tracker/commit/?h=o_noatime&id=cfb41b3abe99ff5e7782071840459646ef259c03
(In reply to comment #0) > Created an attachment (id=197179) [details] [review] > Patch that implements this proposal > > At Tracker we want to open files using O_NOATIME. With GMappedFile this is > currently impossible. Good. Please add this to the commit message; in general the commit message should have everything you wrote in this comment. See https://live.gnome.org/GnomeLove/SubmittingPatches for how to write a good commit message. The first line should look like: GMappedFile: Add API to create from an existing file descriptor > I didn't let g_mapped_file_new reuse g_mapped_file_new_from_fd because > otherwise the error messages couldn't use the display_filename. Hmm. Couldn't you factor out a function which takes a display_filename or NULL for errors? That's a lot of tricky code to basically copy. Also, you need to update glib.symbols. And a test case would be good.
Created attachment 197194 [details] [review] Improved patch based on comments by Colin Walter, Comment #2
Review of attachment 197194 [details] [review]: I like this much better! A few minor comments below. ::: glib/gmappedfile.c @@ +126,3 @@ g_file_error_from_errno (save_errno), + _("Failed to get attributes of file '%s%s%s%s': fstat() failed: %s"), + display_filename ? display_filename : "fd", This would be a bit more work since you'd have to use two separate format strings, but you might give the number of the fd in the error, e.g. "fd 5". Up to you, just pointing out it's possible. ::: glib/gmappedfile.h @@ +38,3 @@ +GMappedFile *g_mapped_file_new_from_fd (gint fd, + gboolean writable, + GError **error) G_GNUC_MALLOC; Should this be under #ifdef G_OS_UNIX ? I don't think it's possible to use on Windows. If you change this you'll also need the #ifdef in glib.symbols.
I thought about this, but to be honest I don't really see the point in displaying the value of the file descriptor in the error message. Is there a good reason? Afaik you can use this new API on Windows. The g_open part in the original code is/was also shared for both Windows and UNIX/traditional POSIX use.
(In reply to comment #5) > I thought about this, but to be honest I don't really see the point in > displaying the value of the file descriptor in the error message. Is there a > good reason? Probably in most situations by the time some developer sees the error message it'd be hard to track down which FD, so yeah, we can skip it. > Afaik you can use this new API on Windows. The g_open part in the original code > is/was also shared for both Windows and UNIX/traditional POSIX use. Sure, filenames work fine on windows. On win32 file descriptors are more special. See this comment in giochannel.h: * On Win32, this can be used either for files opened with the MSVCRT * (the Microsoft run-time C library) _open() or _pipe, including file * descriptors 0, 1 and 2 (corresponding to stdin, stdout and stderr), * or for Winsock SOCKETs. Seems unlikely people get a fd from any of this and want to mmap it, but on the other hand it turns out g_io_channel_unix_new() is also on win32, so we can ship this there too. So - good to commit.
Alright done. Setting this bug as fixed. http://git.gnome.org/browse/glib/commit/?id=ca154c399b879ad17804af5d9a81e607e049861e