Data Saving Patch

Post by Richard Heck
I propose to commit the attached. The effect is: (i) We always create a
backup file in the current directory, but in the backup directory if
"Make Backups" is true; (ii) If we are unable to do so, we ask the user
if s'he wants to proceed; (iii) We delete the backup file after a
successful save, unless "Make Backups" is true.

Does this mean that we delete old backup when saving without backups
enabled?

What's wrong with what I proposed nitially, that is to create the file
under some special name, and rename it to the right name on success?
This means that there is no file to copy.

JMarc

Richard Heck

2014-06-02 19:13:49 UTC

Does this mean that we delete old backup when saving without backups
enabled?

Yes, though that is easily enough fixed.

Post by Jean-Marc Lasgouttes
What's wrong with what I proposed nitially, that is to create the file
under some special name, and rename it to the right name on success?
This means that there is no file to copy.

One could also do it that way. I did it this way only because it
involved changing less code. We already have the backup mechanism in place.

Richard

Richard Heck

2014-06-03 14:44:58 UTC

Does this mean that we delete old backup when saving without backups
enabled?
What's wrong with what I proposed nitially, that is to create the file
under some special name, and rename it to the right name on success?
This means that there is no file to copy.

Try this one, then.

Richard

Jean-Marc Lasgouttes

2014-06-03 15:03:26 UTC

Try this one, then.

This looks good to me, but I did not try it out. I guess we would have
to introduce some abort() in one of the write() functions.

JMarc

Richard Heck

2014-06-03 15:38:29 UTC

Try this one, then.

This looks good to me, but I did not try it out. I guess we would have
to introduce some abort() in one of the write() functions.

Why is that? They all return a boolean to signify whether something went
wrong, which this patch then checks.

I did try it out a few different ways. It seems to work but obviously
should get a bit of testing.

Richard

Jean-Marc Lasgouttes

2014-06-03 15:40:09 UTC

Try this one, then.

This looks good to me, but I did not try it out. I guess we would have
to introduce some abort() in one of the write() functions.

Why is that? They all return a boolean to signify whether something went
wrong, which this patch then checks.

Just to make LyX die from an horrible death and see what happens ;)

JMarc

Richard Heck

2014-06-03 15:46:10 UTC

Try this one, then.

This looks good to me, but I did not try it out. I guess we would have
to introduce some abort() in one of the write() functions.

Why is that? They all return a boolean to signify whether something went
wrong, which this patch then checks.

Just to make LyX die from an horrible death and see what happens ;)

Oh, sorry, I see. I tried that, and a few other possibilities. In that
case, the original file is untouched. A partial file exists at
tmp-filename.lyx, as it should.

rh

Jean-Marc Lasgouttes

2014-06-03 16:02:50 UTC

Post by Jean-Marc Lasgouttes
Just to make LyX die from an horrible death and see what happens ;)

Oh, sorry, I see. I tried that, and a few other possibilities. In that
case, the original file is untouched. A partial file exists at
tmp-filename.lyx, as it should.

Excellent.

JMarc

Georg Baum

2014-06-07 20:42:10 UTC

Does this mean that we delete old backup when saving without backups
enabled?
What's wrong with what I proposed nitially, that is to create the file
under some special name, and rename it to the right name on success?
This means that there is no file to copy.

Try this one, then.

Finally I had some time to do tests and study the code. It works well in
general, but I also found some problems:

- The name of the temp file is very predictable, and does not depend on the
document name. _If_ the user has some problems, and LyX crashes frequently
before the temp file is removed, then you will quickly end up with 100 temp
files. Afterwards, you will not be able to save any file anymore.

- The generation of the temp file name contains a race condiction regarding
another LyX instance saving in the same directory.

- Symlinks do not survive saving anymore

- File permissions do not survive saving anymore. This could lead to
unwanted permission widening if the default permissions for new files are
less restrictive than the particular file you are working on. This was also
the case with the backups already, but these can be switched off.

All problems but the last one are fixed by the attached patch. I reused the
TempFile class for atomic and random temp file name generation to fix the
first two problems. Fixing the last problem correctly is probably more work,
but it is still important IMHO.

Some of my remarks may look like nitpicking. However, we are trying to work
around a problem which does not occur very often (although it causes big
damage if it occurs), and this is a central infrastructure part. Therefore
we have to be extra careful not to destroy the many working use cases (which
might contain subtle details, see e.g. bug 6587).

Georg

Georg Baum

2014-06-07 20:51:58 UTC

Post by Georg Baum
Some of my remarks may look like nitpicking. However, we are trying to
work around a problem which does not occur very often (although it causes
big damage if it occurs), and this is a central infrastructure part.
Therefore we have to be extra careful not to destroy the many working use
cases (which might contain subtle details, see e.g. bug 6587).

And I forgot: Because of this the crash for circle symlinks in my patch
needs to be fixed as well.

Georg

Richard Heck

2014-06-08 20:09:11 UTC

Does this mean that we delete old backup when saving without backups
enabled?
What's wrong with what I proposed nitially, that is to create the file
under some special name, and rename it to the right name on success?
This means that there is no file to copy.

Try this one, then.

Finally I had some time to do tests and study the code. It works well in
- The name of the temp file is very predictable, and does not depend on the
document name. _If_ the user has some problems, and LyX crashes frequently
before the temp file is removed, then you will quickly end up with 100 temp
files. Afterwards, you will not be able to save any file anymore.
- The generation of the temp file name contains a race condiction regarding
another LyX instance saving in the same directory.
- Symlinks do not survive saving anymore
- File permissions do not survive saving anymore. This could lead to
unwanted permission widening if the default permissions for new files are
less restrictive than the particular file you are working on. This was also
the case with the backups already, but these can be switched off.
All problems but the last one are fixed by the attached patch. I reused the
TempFile class for atomic and random temp file name generation to fix the
first two problems. Fixing the last problem correctly is probably more work,
but it is still important IMHO.
Some of my remarks may look like nitpicking. However, we are trying to work
around a problem which does not occur very often (although it causes big
damage if it occurs), and this is a central infrastructure part. Therefore
we have to be extra careful not to destroy the many working use cases (which
might contain subtle details, see e.g. bug 6587).

Nitpicking is exactly what we need here, for the reasons you give. I'd
suggest you go ahead and commit this, and we can deal with the
permissions issue next.

That doesn't seem that bad, does it? We just need to find the original
permissions and then set the new file to have them as well?

By the way, did you see this report?

Post by Georg Baum
I've been getting the following message frequently when doing a normal
save (and I have not been doing anything tricky such as editing the
"Document xyz.lyx has been externally modified. Are you sure you want
to overwrite this file?"
I did not confirm that the above commit is what changed this behavior,
but I thought I would check in to see if anyone else sees this?
Yes, I see this, too.
JÃŒrgen

Did you notice if this still happens with your patch?

Richard

Georg Baum

2014-06-09 09:21:48 UTC

Post by Richard Heck
Nitpicking is exactly what we need here, for the reasons you give. I'd
suggest you go ahead and commit this, and we can deal with the
permissions issue next.

A slightly modified version which does not crash with circular symlinks is
in at bf782ee02ac. Note that circular symlinks are not an issue for this
particular use of copyTo() (such a file cannot be opened anyway), but I
wanted to make sure to avoid such a crash for future users of copyTo.

Post by Richard Heck
That doesn't seem that bad, does it? We just need to find the original
permissions and then set the new file to have them as well?

Yes. However, I believe this can be quite tricky if ACLs come into the play.

Post by Richard Heck
By the way, did you see this report?

Post by Georg Baum
I've been getting the following message frequently when doing a
normal save (and I have not been doing anything tricky such as
"Document xyz.lyx has been externally modified. Are you sure you want
to overwrite this file?"
I did not confirm that the above commit is what changed this
behavior, but I thought I would check in to see if anyone else sees
this?
Yes, I see this, too.
Jürgen

Did you notice if this still happens with your patch?

I saw the report, but I did not see it happening, with or without my patch.
Scott and Jürgen, maybe you try to save on a network file system? Maybe it
has something to do with the checksumming.

Georg

Jürgen Spitzmüller

2014-06-09 09:26:15 UTC

Post by Georg Baum
I saw the report, but I did not see it happening, with or without my patch.
Scott and JÃŒrgen, maybe you try to save on a network file system? Maybe it
has something to do with the checksumming.

No, no network FS, nothing specific here.

JÃŒrgen

Post by Georg Baum
Georg

Jürgen Spitzmüller

2014-06-09 09:47:51 UTC

Post by Georg Baum
I saw the report, but I did not see it happening, with or without my patch.

Post by Georg Baum
Scott and JÃŒrgen, maybe you try to save on a network file system? Maybe it
has something to do with the checksumming.

No, no network FS, nothing specific here.

I tried with latest master (including your recent changes), and I still get
the warning.
* Open document
* Edit
* Save
=> No warning
* Edit more
* Save again
=> This time, the warning dialog pops up.

JÃŒrgen

Post by Georg Baum
JÃŒrgen

Post by Georg Baum
Georg

Kornel Benko

2014-06-09 09:53:29 UTC

Post by JÃ¼rgen SpitzmÃ¼ller

Post by Georg Baum
I saw the report, but I did not see it happening, with or without my patch.

Post by Georg Baum
Scott and JÃŒrgen, maybe you try to save on a network file system? Maybe it
has something to do with the checksumming.

No, no network FS, nothing specific here.

I tried with latest master (including your recent changes), and I still get
the warning.
* Open document
* Edit
* Save
=> No warning
* Edit more
* Save again
=> This time, the warning dialog pops up.

Good recipe. I see it too now.

Post by JÃ¼rgen SpitzmÃ¼ller
JÃŒrgen

Post by Georg Baum
JÃŒrgen

Post by Georg Baum
Georg

Kornel

Georg Baum

2014-06-09 11:27:22 UTC

Post by JÃ¼rgen SpitzmÃ¼ller
I tried with latest master (including your recent changes), and I still
get the warning.
* Open document
* Edit
* Save
=> No warning
* Edit more
* Save again
=> This time, the warning dialog pops up.

I see it as well with this recipe, and fixed it at f792e70d0a. This shows
clearly that there was a gap both in Richards and my testing, otherwise we
would have seen it. Given the fact that there is still a highly nontrivial
change missing to fix the last (known) regression (the file permission
problem) I don't think anymore that saving to a temp file first and then
renaming it is safe enough for 2.1.1. We simply cannot test it enough to
ensure that it does not make the situation worse.

Therefore I vote for taking it out for 2.1.1 (but keep the higher number of
flush()). If you disagree then both bf782ee02a and f792e70d0a should be
backported of course.

Georg

Peter Kümmel

2014-06-09 11:59:56 UTC

I see it as well with this recipe, and fixed it at f792e70d0a. This shows
clearly that there was a gap both in Richards and my testing, otherwise we
would have seen it. Given the fact that there is still a highly nontrivial
change missing to fix the last (known) regression (the file permission
problem) I don't think anymore that saving to a temp file first and then
renaming it is safe enough for 2.1.1. We simply cannot test it enough to
ensure that it does not make the situation worse.
Therefore I vote for taking it out for 2.1.1 (but keep the higher number of
flush()). If you disagree then both bf782ee02a and f792e70d0a should be
backported of course.

Yes, touching 19 files looks a bit too complicated for a fix.

Could you not just ensure that no valid backup/autosave file will be overwritten?

With two backupfile it could look like this:
- write to backup file 1
- write to the settings that backup 1 is latest valid backup file
- next time read from settings which the last valid one was, and use the other
- after successfully writing the backup file update the settings
- then save the master lyx file

When LyX crashs while writing the backup file there is still the last one and the master file.
When LyX crash while writing the settings it will overwrite the latest backup,
but there is still the second latest backup file valid and the master
When LyX then crashs while writing the master lyx file there is at least one
backup file valid.

Post by Georg Baum
Georg

Richard Heck

2014-06-09 14:38:04 UTC

I see it as well with this recipe, and fixed it at f792e70d0a. This shows
clearly that there was a gap both in Richards and my testing, otherwise we
would have seen it. Given the fact that there is still a highly nontrivial
change missing to fix the last (known) regression (the file permission
problem) I don't think anymore that saving to a temp file first and then
renaming it is safe enough for 2.1.1. We simply cannot test it enough to
ensure that it does not make the situation worse.
Therefore I vote for taking it out for 2.1.1 (but keep the higher number of
flush()). If you disagree then both bf782ee02a and f792e70d0a should be
backported of course.

That sounds as if it may be necessary. What about the simpler approach I
used
https://www.mail-archive.com/lyx-***@lists.lyx.org/msg184575.html
there? The basic idea, which could be made even simpler, was just to
copy the
existing LyX file to a new location before saving the new one. Then it
does not
get over-written. JMarc had criticisms of it as implemented that
depended upon
details that could be fixed.

Richard

existing backup file even if b

Georg Baum

2014-06-09 20:08:58 UTC

Post by Georg Baum
Therefore I vote for taking it out for 2.1.1 (but keep the higher number
of flush()). If you disagree then both bf782ee02a and f792e70d0a should
be backported of course.

That sounds as if it may be necessary. What about the simpler approach I
used
there? The basic idea, which could be made even simpler, was just to
copy the
existing LyX file to a new location before saving the new one. Then it
does not
get over-written. JMarc had criticisms of it as implemented that
depended upon
details that could be fixed.

Yes, that looks like it can be implemented more safe.

Georg

Richard Heck

2014-06-09 21:56:16 UTC

The attached patches (i) revert the previous data saving patch for
stable and (ii) implement a different, and safer, approach. There are
some issues here (e.g, with symlinks), but these already exist in the
code that makes user-requested backups. The only real change is to the
code that figures out to what name we should backup the original file.
So this seems about as safe as it could be.

The reason we need this patch for 2.1.x is that the more sophisticated
approach needs more refinement, and testing, that it can get before 2.1.1.

Here's a summary of what this patch does.

If the user has not requested that we backup the original file, we do so
anyway, not to file.lyx~ but to a temporary name of our own creation,
lyxbak-NUM-file.lyx, where NUM is some number we find that gives us a
non-existent file. Note that this filename depends upon which file we
are saving, and we try NUM up to 1024. So we should always be able to
find such a name, unless something very bad has happened.

If we are unable to backup the original file, we issue a warning and ask
the user if s'he wants to continue.

If the file is saved successfully, then we delete our temporary backup
file.

If the file is not saved successfully, then we attempt to move the
backup file back to the original location. If that fails, then we inform
the user that the original file can still be found at a new location.

I put a "return false" at the beginning of Buffer::writeFile() and
verified that the original file is properly restored. Since we move the
file back and forth unless it is a symlink, this should preserve
permissions as well.

Comments?

Richard

Pavel Sanda

2014-06-10 06:50:24 UTC

There are some issues (e.g, with symlinks)

So are the symlinks preserved or not with this patch?

Pavel

Richard Heck

2014-06-10 14:12:59 UTC

There are some issues (e.g, with symlinks)

So are the symlinks preserved or not with this patch?

The exact same thing happens as if backups are enabled and the save
fails: If the save works, the new file is still a symlink; if the save
fails, then, since the backup was made by copying, the restored file is
not a symlink, but a copy.

Richard

Pavel Sanda

2014-06-10 15:35:59 UTC

There are some issues (e.g, with symlinks)

So are the symlinks preserved or not with this patch?

If the save works, the new file is still a symlink; if the save fails,
then, since the backup was made by copying, the restored file is not a
symlink, but a copy.

I see, thanks. P

Richard Heck

2014-06-11 21:08:50 UTC

There are some issues (e.g, with symlinks)

So are the symlinks preserved or not with this patch?

If the save works, the new file is still a symlink; if the save fails,
then, since the backup was made by copying, the restored file is not a
symlink, but a copy.

I see, thanks.

Since I see no other objection, I have committed this.

Richard

Georg Baum

2014-06-13 19:36:10 UTC

There are some issues (e.g, with symlinks)

So are the symlinks preserved or not with this patch?

I see, thanks.

Since I see no other objection, I have committed this.

Finally I had some time to look at it, and don't have any objections either,
but it looks like you forgot to push;-)

Georg

Richard Heck

2014-06-14 21:43:00 UTC

There are some issues (e.g, with symlinks)

So are the symlinks preserved or not with this patch?

I see, thanks.

Since I see no other objection, I have committed this.

Finally I had some time to look at it, and don't have any objections either,
but it looks like you forgot to push;-)

Apparently so...

rh

Scott Kostyshak

2014-06-09 09:48:09 UTC