GNOME Bugzilla – Bug 540295
Local storage renames do not delete folder summaries for VFAT fs when running in a non Windows OS
Last modified: 2013-09-14 16:52:46 UTC
Please describe the problem: This problem occurs when trying to rename a folder in a local storage which does not support hard links like VFAT. The code in camel-store-summary is: } else if (link(old, new) == 0 /* and link for files */ || (stat(new, &st) == 0 && st.st_nlink == 2)) { if (unlink(old) == 0) { ret = 0; } else { err = errno; unlink(new); ret = -1; } This is assuming that link will work for any non-window OS and this is false if a GNU/Linux OS is storing the folder summaries in a VFAT filesystem, Steps to reproduce: 1- Setup a maildir folder in a VFAT filesystem in a GNU/linux OS 2- Rename the folder Actual results: The old summary file is not deleted Expected results: The old summary file must be deleted Does this happen every time? Yes Other information:
Created attachment 113454 [details] [review] Patch that removes the old summaries This patch always perform a rename which will the rename succeed even if the folder summary is located in a FS that does not support link().
Matt, can you review it?
Seems okay, though I wish I better understood why it was written that way to begin with. I don't understand the benefit of using link()/unlink() versus rename() for files. The Win32 logic seems a lot more straight-forward. Would it be feasible to just use that for all platforms? This patch gets us pretty close to that anyway.
if you are going to use rename(), the stat() is unnecessary (it's only purpose was to check the link count...) the only reason I can think of for using link()/unlink() vs rename() is for making sure you don't clobber an existing file (with the same name as newname) not sure if that's the reason it was originally written like that or not. also not sure if that's a valid concern.
(In reply to comment #3) > Seems okay, though I wish I better understood why it was written that way to > begin with. I don't understand the benefit of using link()/unlink() versus > rename() for files. The only reason I could think about is that it avoids sometimes unnecessary system calls although I don't see a big benefit on that. I also don't understand pretty well the check for num links == 2. > The Win32 logic seems a lot more straight-forward. Would it be feasible to > just use that for all platforms? This patch gets us pretty close to that > anyway. I think we could use it for all platforms as you say.
Another option is to use g_file_move(), which is a higher-level GIO function that should handle renames appropriately for the local filesystem, in case there's any other weird corner cases we're not considering.
the st_links == 2 seems like broken logic (it should have stat()'d the original file and checked that the number of links increased by 1 I guess). but the idea was to check that the link() actually worked (might have been to work around NFS issues?) unfortunately checking st_links seems like a race to me...
Guys any update on this? Would you accept a new patch with Win32 logic for all platforms? Thx!
Sergio, if it is going to be specific to win32, them may be you can #ifdef it for Windows alone..
(In reply to comment #9) > Sergio, if it is going to be specific to win32, them may be you can #ifdef it > for Windows alone.. > Oh not at all, what I said is what Matthew suggested in comment #3, and is to use the code of Win32 for all platforms because is straightforward and platform-independent.
Merging the logic seems like the way to go, since the current code has no comments explaining the significance of the differences. Submit a revised patch and we'll have a look at it. Subversion history would probably tell the story, but my guess is the POSIX code was written first, an issue with it was discovered on Windows (perhaps even the same VFAT issue you're reporting), so Tor or someone came in and added the simpler WIN32 version.
Created attachment 115549 [details] [review] Patch that removes the old summaries This new patch simplifies the folder renaming. Basically what it does is to remove any platform specific code and uses the glib functions to carry out.
Looks fine to me. Please commit.
(In reply to comment #13) > Looks fine to me. Please commit. > I don't have permissions to do so. Should I ask for an account?
2008-07-31 Matthew Barnes <mbarnes@redhat.com> ** Fixes bug #540295 * camel-local-store.c (xrename): Use g_rename() for all platforms. Previous POSIX logic didn't work for VFAT filesystems. (Patch by Sergio) Committed to trunk (revision 9244).