GNOME Bugzilla – Bug 680326
thumbnailer should not modify atime of file
Last modified: 2013-06-07 20:20:31 UTC
The thumbnailer should not modify the atime of file. input_stream = G_INPUT_STREAM (g_file_read (file, NULL, NULL)); g_local_file_read doesn't set O_NOATIME. I guess we should open the file ourselves and pass the fd.
Created attachment 219343 [details] [review] Try not to modify atime on files while thumbnailing
Where is _open_fd() defined? Did you have it in another previous patch maybe?
Created attachment 219346 [details] [review] Try not to modify atime on files while thumbnailing
Review of attachment 219346 [details] [review]: Is this really necessary? We won't update the atime on these files in linux anyway, since relatime is the default mount option, and you only do it in the linux codepath. ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +27,3 @@ +#ifndef _GNU_SOURCE +#define _GNU_SOURCE +#endif this should go in configure.ac as AC_USE_SYSTEM_EXTENSIONS @@ +314,3 @@ + g_return_val_if_fail (path != NULL, -1); + +#if defined(__linux__) I think i'd rather HAVE_ONOATIME than __linux__ @@ +316,3 @@ +#if defined(__linux__) + fd = g_open (path, O_RDONLY | O_NOATIME, 0); + if (fd == -1 && errno == EPERM) this should have a comment saying "O_NOATIME isn't allowed if we don't own the file"
Created attachment 220914 [details] [review] use gs_file_read_noatime from libgsystem
Review of attachment 220914 [details] [review]: ::: autogen.sh @@ +31,3 @@ +if test ! -f libgnome-desktop/libgsystem/README; then + echo "+ Setting up submodules" + git submodule init isn't git subtree en vogue now? No idea which one is better, just heard people talking about subtree recently. ::: libgnome-desktop/Makefile.am @@ +19,3 @@ +libgsystem_srcpath := libgsystem +libgsystem_cflags = $(GNOME_DESKTOP_CFLAGS) +libgsystem_libs = $(GNOME_DESKTOP_LIBS) Why does it need the cflags/libs of the parent ? ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +347,2 @@ if (input_stream == NULL) { + input_stream = gs_file_read_noatime (file, NULL, NULL); cool, that's easy.
(In reply to comment #6) > Review of attachment 220914 [details] [review]: > > ::: autogen.sh > @@ +31,3 @@ > +if test ! -f libgnome-desktop/libgsystem/README; then > + echo "+ Setting up submodules" > + git submodule init > > isn't git subtree en vogue now? No idea which one is better, just heard people > talking about subtree recently. I'd never heard of it until now, but: https://github.com/apenwarr/git-subtree/blob/master/THIS-REPO-IS-OBSOLETE Basically only in newer git versions, and only if you have some "contrib" thing enabled. > ::: libgnome-desktop/Makefile.am > @@ +19,3 @@ > +libgsystem_srcpath := libgsystem > +libgsystem_cflags = $(GNOME_DESKTOP_CFLAGS) > +libgsystem_libs = $(GNOME_DESKTOP_LIBS) > > Why does it need the cflags/libs of the parent ? For Gio basically. It doesn't hurt really to over-link it since it just ends up as part of the same .so anyways.