Sat 03 Nov 2007 10:00:20 PM UTC, comment #17:
>This seems to work fine.
Thanks, I checked it in.
|
Sat 03 Nov 2007 08:49:59 PM UTC, comment #16:
>The only concern I have is that there are potential localisation
>problems.
Definitely. I cross-posted a question about this to bug-gnu-gettext and pspp-dev. Maybe we'll get some good suggestions.
|
Sat 03 Nov 2007 07:58:39 AM UTC, comment #15:
This seems to work fine.
The only concern I have is that there are potential localisation problems. Constructs like
msg (SE, _("Can't read from %s as a %s because it is "
"already being read as a %s."),
fh_get_name (h), type, a->type);
will cause complaints from translators, because it may well prove to be impossible to form correct sentences in their language.
In particular, languages which have noun gender will balk at the English "a %s", because the pronoun has to agree with the noun, but the translator can't possibly know what the noun will be.
|
Sat 03 Nov 2007 02:28:56 AM UTC, comment #14:
I noticed that I posted a slightly old version of overwrite-input-file-allow.patch. Here is an updated version.
(file #14285)
|
Fri 02 Nov 2007 05:54:07 AM UTC, comment #13:
OK, at long last, here is what should be a real fix.
The first patch, overwrite-input-file-allow.patch, is the straightforward part: make reading and writing separate for locking and do atomic replacement of files.
Unfortunately, this doesn't really work, because the file handle architecture is based on inode number, with the assumption that a file won't get deleted and then a new file created under a different name with the same inode number. This is a bad assumption, and at least on my system with an ext3 file system it causes the match-files.sh test to consistently fail.
Thus, the second patch and much larger patch, patches/identity-stability.patch, becomes necessary. In this patch, we drop the association between a file handle and an inode number. Instead, a file handle just represents a file name, and we do inode number lookup and locking on-demand using a separate table of new "struct fh_lock" structures. This fixes the problem and allows everything to pass.
Comments?
(file #14275, file #14276)
|
Sun 21 Oct 2007 06:00:02 AM UTC, comment #12:
I now have a fix for this problem. Unfortunately it breaks other things. I'm working on a fix for that :-)
|
Sat 13 Oct 2007 05:07:58 AM UTC, comment #11:
>You're right. It probably won't gain us anything. And it would be
>an uncommon situation anyway.
OK. The solution should be straightforward. I will work on it.
|
Fri 12 Oct 2007 10:03:46 AM UTC, comment #10:
You're right. It probably won't gain us anything. And it would be an uncommon situation anyway.
|
Thu 11 Oct 2007 03:41:31 PM UTC, comment #9:
>There's no guarantee however that you'll have permission to create
>arbitrary files in the directory of the input file. Perhaps we
>should fall back to /tmp, if the first attempt fails, and hope
>that it's on the same filesystem.
Does that solve anything? To create a new file we need write permission in the new file's directory. To rename over an existing file, we need write permission in the existing file's directory. No?
The only way we could avoid needing write permission in the file's directory, I think, is by opening the existing file and truncating it to length 0. But then we'd lose the contents that we need to read. I guess we could copy the file's contents to a temporary file and then truncate and overwrite it, but I am not sure that that is really a winner for performance.
I do not think it is a common situation to want to write to a file in a directory in which one does not have write permission. Do you have a scenario in mind?
|
Thu 11 Oct 2007 10:30:48 AM UTC, comment #8:
It sounds plausible. There's no guarantee however that you'll have permission to create arbitrary files in the directory of the input file. Perhaps we should fall back to /tmp, if the first attempt fails, and hope that it's on the same filesystem.
|
Wed 10 Oct 2007 03:29:44 PM UTC, comment #7:
OK. This is do-able. There is an issue to be resolved, however: when does the file get replaced? This comes up when an error occurs while the operation is running. In that case, one would hope that the original file would not be deleted.
One solution is: when a file is opened for writing, create it under a temporary name other than the one that the user chose. Then, when the file is closed, rename it to the final name. (The temporary and final files should be in the same directory to ensure that rename does not fail because it is a cross-file system operation.)
Does this sound reasonable to you?
|
Wed 10 Oct 2007 10:16:44 AM UTC, comment #6:
The situation is ....
System files no longer get truncated. This is good.
Your test shows that the system behaves as you intend, but not as I expected. I expected the syntax
get file='foo.sav'.
save outfile='foo.sav'.
to run without error, whereas your test expects it to produce an error. This syntax runs without error under spss as does the slightly more useful example:
get file='foo.sav'.
compute y = x.
save outfile='foo.sav'.
In any case, I imagine that most gui users would expect to be able to: open a file, add some data and/or variables, click "Save" and have the new data written back to the existing file without error.
|
Tue 09 Oct 2007 01:20:27 PM UTC, comment #5:
>A simple syntax...where foo.sav is an existant non-empty system
>file shows there is still a problem.
I can't reproduce the problem by hand and, furthermore, the test that I checked in that tests for it passes for me (as long as I chmod +x by hand).
|
Tue 09 Oct 2007 12:52:24 PM UTC, comment #4:
>It seems to do the business.
I have to withdraw that comment.
A simple syntax
GET /FILE='foo.sav'.
SAVE /OUTFILE='foo.sav'.
where foo.sav is an existant non-empty system file shows there is still a problem.
|
Tue 09 Oct 2007 03:50:21 AM UTC, comment #3:
>It seems to do the business.
Glad to hear it. I checked this in.
|
Mon 08 Oct 2007 11:08:23 AM UTC, comment #2:
Thanks Ben.
It seems to do the business.
|
Mon 08 Oct 2007 01:04:49 AM UTC, comment #1:
It's actually a bug in the system file and portable file writers. Here's a fix and a test.
(file #14112)
|
Sun 07 Oct 2007 09:26:23 AM UTC, original submission:
1. Open an existing *.sav file.
2. Modify it.
3. Click Save.
You'll get a error message "Can't open file x for writing because it's already open for reading". And then the file has a length of zero.
|