GNOME Bugzilla – Bug 786831
g_file_replace should not fail to replace a readonly file when passed G_FILE_CREATE_REPLACE_DESTINATION
Last modified: 2018-05-24 19:46:58 UTC
Currently g_file_replace() fails when the file being replaced is readonly (eg. 0444 mode), this should not happen when the flag G_FILE_CREATE_REPLACE_DESTINATION is passed, as docs for that flag says: "Replace the destination as if it didn't exist before. Don't try to keep any old permissions, replace instead of following links. This is generally useful if you're doing a "copy over" rather than a "save new version of" replace operation. You can think of it as "unlink destination" before writing to it, although the implementation may not be exactly like that. Since 2.20" This is bug is blocking gedit/gtksourceview bug 644358 . I will be attaching a patch for this issue.
Created attachment 358466 [details] [review] glocaloutputstream.c: don't fail when replacing readonly file Don't fail at replacing a readonly file when G_FILE_CREATE_REPLACE_DESTINATION flag is passed. Add tests for this g_file_replace() failing case.
Adding block for bug 644358, gedit/gtksourceview just need to pass G_FILE_CREATE_REPLACE_DESTINATION to be fixed on their side, I will post patches for them in the mentioned bug.
Review of attachment 358466 [details] [review]: ::: gio/glocalfileoutputstream.c @@ +789,3 @@ + { + /* In case of overwriting a readonly file, we need + * to open with O_RDONLY to allow for that case. */ s/to allow for that case/to allow the original file to be opened/ ::: gio/tests/file.c @@ +804,3 @@ +test_replace_readonly (void) +{ + GFile *replacedfile; Please use underscores in function names, i.e. replaced_file, ratherthanrunningallthewordstogether. @@ +809,3 @@ + GFileOutputStream *outs; + GError *local_error = NULL; + GError **error = &local_error; Drop `error` and just pass `&local_error` to all the functions below — it’s clearer that way. @@ +811,3 @@ + GError **error = &local_error; + guint32 romode = S_IFREG | 0444; + gboolean make_backup; I’d also like to see another two variants of this test which check whether it works when the file being replaced is a symlink (with and without backups being created). @@ +815,3 @@ + g_test_bug ("786831"); + + replacedfile = g_file_new_tmp ("tmp-replaced-readonlyXXXXXX", This code is duplicated for the make_backup=FALSE and make_backup=TRUE tests. It would be better to factor it out into another function which takes make_backup as an argument, to avoid the code duplication.
Created attachment 359920 [details] [review] glocaloutputstream.c: don't fail when replacing readonly file Don't fail at replacing a readonly file when G_FILE_CREATE_REPLACE_DESTINATION flag is passed. Add tests for this g_file_replace() failing case. -------- Updated patch for review comments.
Review of attachment 359920 [details] [review]: A few issues with the unit test. ::: gio/tests/file.c @@ +803,3 @@ +static void +test_replace_readonly_file (gboolean make_backup, + gboolean symlink) Nitpick: I’d call this `use_symlink` to make it more obvious the variable is a boolean controlling the test, rather than a GFile representing a symlink (for example). @@ +825,3 @@ + ro_file = g_file_new_tmp ("tmp-replaced-readonlyXXXXXX", + &iostream, &local_error); + g_assert (ro_file != NULL); Use `g_assert_nonnull (ro_file);` and move it below the g_assert_no_error() call so you get more detailed failure information if it does fail. @@ +840,3 @@ + { + gchar *symlink_path; + gchar *points_to; Should be `const`. @@ +845,3 @@ + symlink_path = g_strdup_printf ("%s_SYMLINK", points_to); + symlink_file = g_file_new_for_path (symlink_path); + g_assert (symlink_file != NULL); There’s no point in this g_assert() call; we know g_file_new_for_path() always returns a non-NULL object instance. @@ +848,3 @@ + /* Create a symlink pointing to readonly file */ + ret = g_file_make_symbolic_link (symlink_file, points_to, NULL, &local_error); + g_assert_cmpint (ret, ==, TRUE); Use `g_assert_true (ret);` rather than g_assert_cmpint() and put it after the g_assert_no_error() call, so that more detailed failure information can be outputted from the GError if the test does fail. @@ +864,3 @@ + + if (local_error != NULL || outs == NULL) + g_test_fail (); This g_test_fail() branch seems to be a duplicate of the two g_assert_*() calls below. Drop it. @@ +866,3 @@ + g_test_fail (); + g_assert_no_error (local_error); + g_assert (outs != NULL); Use `g_assert_nonnull (outs);` to get more detailed failure information. @@ +873,3 @@ + ret = g_output_stream_write_all (G_OUTPUT_STREAM (outs), buf, buf_len, + &bytes_written, NULL, &local_error); + g_assert_cmpint (ret, ==, TRUE); Use `g_assert_true (ret);` rather than g_assert_cmpint() and put it after the g_assert_no_error() call, so that more detailed failure information can be outputted from the GError if the test does fail. @@ +875,3 @@ + g_assert_cmpint (ret, ==, TRUE); + g_assert_no_error (local_error); + g_assert (bytes_written == buf_len); Use `g_assert_cmpuint (bytes_written, ==, buf_len);` to get more detailed failure information. @@ +885,3 @@ + replaced_file_path = g_file_get_path (replaced_file); + ret = g_file_get_contents (replaced_file_path, &contents, &contents_len, &local_error); + g_assert_cmpint (ret, ==, TRUE); Use `g_assert_true (ret);` rather than g_assert_cmpint() and put it after the g_assert_no_error() call, so that more detailed failure information can be outputted from the GError if the test does fail.
Created attachment 359982 [details] [review] glocaloutputstream.c: don't fail when replacing readonly file Don't fail at replacing a readonly file when G_FILE_CREATE_REPLACE_DESTINATION flag is passed. Add tests for this g_file_replace() failing case. -------------------- Updated patch from review comments.
Review of attachment 359982 [details] [review]: Looks good to me. Ondrej, does it look good to you?
Review of attachment 359982 [details] [review]: I was convinced that G_FILE_CREATE_REPLACE_DESTINATION is just to avoid following of symlinks, but it seems it is rather helper for file managers when copying files as per the documentation, so it makes sense to me... (I suppose that also some gvfs backends may need some fixes in that case). ::: gio/glocalfileoutputstream.c @@ +786,3 @@ #endif + if (fd == -1 && (flags & G_FILE_CREATE_REPLACE_DESTINATION)) Why not set the necessary flags immediately at the beginning of the function and update the comment "We only need read access to the original file if we are creating a backup.", which is a pretty misleading, because O_RDWR, O_WRONLY is used immediately after it...
(In reply to Ondrej Holy from comment #8) > Review of attachment 359982 [details] [review] [review]: > [snip] > > + if (fd == -1 && (flags & G_FILE_CREATE_REPLACE_DESTINATION)) > > Why not set the necessary flags immediately at the beginning of the function > and update the comment "We only need read access to the original file if we > are creating a backup.", which is a pretty misleading, because O_RDWR, > O_WRONLY is used immediately after it... Yes, comment is misleading, create_backup needs write as it uses ftruncate later on. But, if G_FILE_CREATE_REPLACE_DESTINATION is passed, then make_backup does not use ftruncate as takes another code path, that's why we can use O_WRONLY for make_backup when going together with G_FILE_CREATE_REPLACE_DESTINATION. I'm attaching a new patch with that approach. Tests passed succesfully.
Created attachment 360277 [details] [review] glocaloutputstream.c: don't fail when replacing readonly file Don't fail at replacing a readonly file when G_FILE_CREATE_REPLACE_DESTINATION flag is passed. Add tests for this g_file_replace() failing case.
ping
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1285.