Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

skicka: error creating Google Drive client: rename /tmp/skicka.metadata668194580 /usr/home/me/.skicka.metadata.cache: cross-device link #64

Closed
bdklahn opened this issue Apr 7, 2015 · 10 comments

Comments

@bdklahn
Copy link

bdklahn commented Apr 7, 2015

That's one thing a recent update broke, for me. My OS (PC-BSD) is on a ZFS mirror, and I think /tmp and /usr/home/ might be on different data sets. FreeBSD might not allow cross-device links, by default.
See: http://golang.org/pkg/os/#Rename
"OS-specific restrictions might apply."

Maybe the temp file could be made in the home dir. I see you create and use a userHomeDir() function in skicka.go. If that were included in gdrive.go, maybe you could do:

f, err := ioutil.TempFile(userHomrDir(), "skicka.metadata")

I don't know if it makes sense to include a skicka.go function in gdrive.go, or if a separate utilities library file is in order, to serve both.

I realize this might not affect many others. But this is a little like download utilities: They often keep the temporary partial download files in the same directory, as the completed download; not in the system temp dir.

The temp file is worthless if something goes wrong (unless, maybe, for debugging), and perhaps ought to be deleted on error, anyway.
"It is the caller's responsibility to remove the file when no longer needed." (http://golang.org/pkg/io/ioutil/#TempFile)
I guess that is moot if the rename works, but . . .

One odd thing is that I can do that same rename (mv) from my bash command line. So I don't know if golang's os.rename is using the same library, or if the OS packaged console apps are afforded more permission, or . . .

@bdklahn bdklahn changed the title skicka: error creating Google Drive client: rename /tmp/skicka.metadata668194580 /usr/home/b/.skicka.metadata.cache: cross-device link skicka: error creating Google Drive client: rename /tmp/skicka.metadata668194580 /usr/home/me/.skicka.metadata.cache: cross-device link Apr 7, 2015
mmp pushed a commit that referenced this issue Apr 8, 2015
@mmp
Copy link
Contributor

mmp commented Apr 8, 2015

I think that change should fix it; let me know.

Thanks!

@bdklahn
Copy link
Author

bdklahn commented Apr 8, 2015

commit af59d86
. . . does seem to have fixed the issue.

Before I ran skicka, I took a look at the diff and saw that you copy the file from the sys temp dir to a local temp file, if the rename from the sys temp dir doesn't work. I had thought about something like this, but . . .

  1. Just as the create/write of the temporary file can go wrong, there's a small chance that the write from a copy might too (I guess that's why you copy to the local temp file first).
  2. It's probably good practice to then clean up any temp files created in the sys temp dir.

So I thought that defaulting to creating the temp file in the home dir, in the first place, might be the simplest/safest. Also you find out sooner, rather than later, whether the cache file will be writable to the location you want to access it from (useful), before going through the trouble of creating it.

But, anyway, this works. And I see you've been busy on probably more impactful commits. :-)
I guess, if this mattered that much to me, it would almost be as much work for a modify and pull request, than a comment, here. :-)

Thanks!
Best

@andapop
Copy link

andapop commented Apr 9, 2015

@bdklahn I was having the exact same problem. So glad you opened the issue!
This fix came just in time :D
Thank you so much!!!

@mmp mmp closed this as completed Apr 9, 2015
@david50407
Copy link

I got a similar problem on Windows,

skicka: error creating Google Drive client: rename C:\Users\Davy\.skicka.metadata447444166  C:\Users\Davy/.skicka.metadata.cache: Cannot create a file when that file already exists.

And I'm using the version after this patch.

Does anyone have idea?

@david50407
Copy link

And I find the same issue when using os.Rename on Windows here: tv42/go-datastore@4e96acd

I'll create a new issue here.

mmp pushed a commit that referenced this issue Apr 27, 2015
@mmp
Copy link
Contributor

mmp commented Apr 27, 2015

It looks like Windows doesn't like renaming a file on top of an existing one. I added a case to remove the target file in that case; please let me know if this fixes it.

@mmp mmp reopened this Apr 27, 2015
@david50407
Copy link

I got File in use error when remove the target file on Windows.

BTW, my /tmp is full with creating too many skicka.metadataXXXXXX files on my Linux.
I think that commit af59d86 do not remove the metadata temporary file in /tmp when we use the 'Plan B'.

@bgrainger
Copy link
Contributor

@david50407 That error sounds very much like issue #75 that I just opened. (I have a local fix for that but haven't pushed it yet.)

@david50407
Copy link

@bgrainger Yes, I think that's the same one.

@mmp
Copy link
Contributor

mmp commented May 1, 2015

Closing this as I believe that it should also be fixed by 93fb2d9.

@mmp mmp closed this as completed May 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants