Thanks a bunch for the comments. Tammo Some inline responses: On Wed, Mar 08, 2006 at 09:58:52PM -0500, Morten Welinder wrote:
Very interesting. Some comments: 1. Bugzilla is a far better place. Please open a bug and attach the patch.
Done, I submitted a patch that also includes the changes described in this mail.
2. The guts of this ought to live in goffice, probably go-file.c
Not sure I see this argument. The "guts" primarily are specialized to the semantics of gnumeric, doing things like adjusting gui properties. I agree that probably some of the file operations could move there, but it seems cleaner to keep it together.
3. I don't think the mechanism to lock should be O_EXCL
because it does not work with NFS. Symlinks would
work there, but might be an issue on Win32.
O_EXCL is said to (and actually appears to) work fine on modern kernels (>= 2.6.5).
4. Please use g_get_host_name if available. Alway use g_get_user_name.
Done, see attached incremental patch.
5. You need to ensure that other people can read the lock
files.
Ah, I suppose you mean setting the umask... Done.
6. If $EVIL creates a 1GB fake lock file, will Gnumeric crash?
Not planning to test this one ;) I was using g_get_file_contents, which I would hope does not simply crash when files are too big... The attached new patch explicitly reads only a bounded amount.
7. What about character set for the file? Since the encoding of the username is
not known, it appears that we can end up trying to display non-UTF8 text.
(Same issue if $EVIL puts random binary stuff into a lockfile.)
New patch should address this.
8. You are writing a zero byte at the end of the file. Worse, it looks like you
depend on it being there when you read the file.
Should be fixed now.
9. Things need to be translated, i.e., use _() as appropriate.
Done.
Morten
Attachment:
gnumeric.dotlocking-v1.0.patch
Description: Text document