google / skicka

Command-line utility for working with Google Drive. Join the mailing list at https://groups.google.com/forum/#!forum/skicka-users.
Apache License 2.0
1.3k stars 118 forks source link

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

Closed bdklahn closed 9 years ago

bdklahn commented 9 years ago

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 . . .

mmp commented 9 years ago

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

Thanks!

bdklahn commented 9 years ago

commit af59d8657153320c700ad7501cb058b8dd86dba1 . . . 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 commented 9 years ago

@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!!!

david50407 commented 9 years ago

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 commented 9 years ago

And I find the same issue when using os.Rename on Windows here: https://github.com/tv42/go-datastore/commit/4e96acd04a18c4f3aedacc7f474ab725d579cd19

I'll create a new issue here.

mmp commented 9 years ago

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.

david50407 commented 9 years ago

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 https://github.com/google/skicka/commit/af59d8657153320c700ad7501cb058b8dd86dba1 do not remove the metadata temporary file in /tmp when we use the 'Plan B'.

bgrainger commented 9 years ago

@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 commented 9 years ago

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

mmp commented 9 years ago

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