Bug#2531: PATCH: Make install-info use fcntl (via perl's flock) for locking
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Ian Jackson wrote: merge 2531 3410 thanks Nathanael Nerode writes (Re: PATCH: Make install-info use fcntl (via perl's flock) for locking): unmerge 3410 thanks # 3410 is a different bug, requesting that the lockfile be # cleaned up on process kill The only correct fix to that is to use fcntl instead, so it should remain merged. In your opinion. If that's the opinion of the current dpkg maintainers, fine, but you're not one of them! I happen to be quite certain that you're wrong. 3410 could be fixed correctly by using flock(2) or lockf(2). Or (better) by implementing a completely lockless algorithm, such as the full-database-rebuild -- which is apparently the preferred choice. Also, the correct way to describe this bug relationship nowadays is with a block: unmerge 3410 2531 block 3410 with 2531 They are *not* the same bug, even if the best fix for one is to fix the other (which it isn't). The long-term plan, anyway, is to eliminate the current install-info in favor of one which rebuilds the index from scratch (a very good idea), which will solve 3410 and render 2531 a 'wontfix' wishlist bug. I would advise unmerging the bugs, marking 2531 'wontfix' and attempting to solve 3410 with the lockless algorithm, if any dpkg maintainers are listening. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.3 (GNU/Linux) iD8DBQFEmEAyRGZ0aC4lkIIRAkN5AJ0Vyci93kWzfqrNLwi2yRbCkbMpDACeI96q s/Pi+1JBufSeWCdBZILein8= =hgss -END PGP SIGNATURE- -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#2531: PATCH: Make install-info use fcntl (via perl's flock) for locking
merge 2531 3410 thanks Nathanael Nerode writes (Re: PATCH: Make install-info use fcntl (via perl's flock) for locking): unmerge 3410 thanks # 3410 is a different bug, requesting that the lockfile be # cleaned up on process kill The only correct fix to that is to use fcntl instead, so it should remain merged. Hmm. In that case, what is the *point* of using fcntl locking? *EVER*? Fcntl locking is a fscking waste of time and effort in this case. Is it simply to make sure that the lock file is deleted if the program dies? Yes (well, there are other advantages too but that's the main one). This can be done in a much simpler way, without a lock-method transition, by catching kill signals and deleting the lock file on all exit paths. As suggested in bug 3410, which was mistakenly merged with this one. This is not reliable. I'm afraid I don't have time right now to explain all of this to you but all of the stuff I've been explaining should be well-known to anyone messing around in this area. Regards, Ian. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#2531: PATCH: Make install-info use fcntl (via perl's flock) for locking
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 unmerge 3410 thanks # 3410 is a different bug, requesting that the lockfile be # cleaned up on process kill Ian Jackson wrote: tags 2531 - patch thanks Nathanael Nerode writes (PATCH: Make install-info use fcntl (via perl's flock) for locking): This patch converts install-info to use perl's flock for locking. ... As part of this change, it now operates directly on the dir file Unfortunately, this is wrong. If you operate directly on the dir file then disk full and other errors can cause the file to be truncated. This is OK if you write the new version to a temp file and rename into place, because only the temp file is damaged. If you try to do it by writing to the primary file directly then if your script fails and the primary file is damaged, the next run will probablu overwrite the backup. For this reason, when using fcntl locking, you generally have to use a _separate_ lockfile. Hmm. In that case, what is the *point* of using fcntl locking? *EVER*? Fcntl locking is a fscking waste of time and effort in this case. Is it simply to make sure that the lock file is deleted if the program dies? This can be done in a much simpler way, without a lock-method transition, by catching kill signals and deleting the lock file on all exit paths. As suggested in bug 3410, which was mistakenly merged with this one. I see absolutely no other advantage to using fcntl locking; none at all. Therefore I strongly advise closing the bug immediately as wontfix. You're the submitter, so I advise that you do. Also, when changing between different locking methods, you have to either be sure that only your program might be editing the file, Done. If the lockfile exists, my program dies immediately, before touching the file. Yes, simplistic, but two versions of install-info *shouldn't* be running at the same time thanks to the non-parallelism of dpkg in general, so this is strictly a corner case. As is this entire bug, until dpkg becomes more parallel. Of course, the long-term solution is to regenerate the dir file fresh from sources each time, in which case it doesn't matter whether it gets trashed by one run. Unfortunately this doesn't appear to be quite possible yet. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.3 (GNU/Linux) iD8DBQFEi1akRGZ0aC4lkIIRAgVlAJ48NtlZ6Q9+7oyOk/9Mhy92ApA8VwCaAx8H 4AqBnCNjL5LYsaEZYxNvNA4= =2zOM -END PGP SIGNATURE- -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#2531: PATCH: Make install-info use fcntl (via perl's flock) for locking
tags 2531 - patch thanks Nathanael Nerode writes (PATCH: Make install-info use fcntl (via perl's flock) for locking): This patch converts install-info to use perl's flock for locking. ... As part of this change, it now operates directly on the dir file Unfortunately, this is wrong. If you operate directly on the dir file then disk full and other errors can cause the file to be truncated. This is OK if you write the new version to a temp file and rename into place, because only the temp file is damaged. If you try to do it by writing to the primary file directly then if your script fails and the primary file is damaged, the next run will probablu overwrite the backup. For this reason, when using fcntl locking, you generally have to use a _separate_ lockfile. Also, when changing between different locking methods, you have to either be sure that only your program might be editing the file, or you have to have a dual running approach. In this case one possibility would be to use the .lock file that current install-info uses as the separate lockfile and to write `perl-flock' into it to indicate that the lock is held either (a) by the process which created the file empty or (b) if the file contains `perl-flock' by the process with a Perl flock on the file. So the process of locking and updating would be: 1. Attempt to open .lock for writing O_EXCL a. If this succeeds, write perl-flock\n b. If this fails, i. open the file for reading; if this fails ENOENT go back to start ii. check whether the file contains only perl-flock\n and if not sleep 1 and go back to start 2. Lock .lock with Perl's flock 3. Read old file contents 4. Write new file contents to temporary file 5. Unlink old backup file 6. link(2) old file to backup file 7. rename(2) temporary file to primary file Ian. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#2531: PATCH: Make install-info use fcntl (via perl's flock) for locking
Ian Jackson writes (Re: PATCH: Make install-info use fcntl (via perl's flock) for locking): So the process of locking and updating would be: 1. Attempt to open .lock for writing O_EXCL a. If this succeeds, write perl-flock\n b. If this fails, i. open the file for reading; if this fails ENOENT go back to start ii. check whether the file contains only perl-flock\n and if not sleep 1 and go back to start 2. Lock .lock with Perl's flock 3. Read old file contents 4. Write new file contents to temporary file 5. Unlink old backup file 6. link(2) old file to backup file 7. rename(2) temporary file to primary file I should point out that the lack of explicit unlocking and deletion here is deliberate; the kernel will release the fcntl lock for us. It does mean that after any new program has taken out the lock in this way all old programs will fail thinking the lock is already held. (Also, there is a bug: in 1(b)(i) you have to open the file O_RDWR without O_CREAT.) If this is not acceptable, then you have to unlink the .lock file as part of unlocking. When you have this protocol, acquiring the lock is more complex, to avoid the race where you fcntl lock the lockfile just as the previous lock holder is unlinking it. You hold the lock only if after acquiring the fcntl lock you fstat the file to check the file you've fcntl locked is still named as the lockfile, and you may not unlink the lockfile without holding the lock: 1. Attempt to open .lock for writing O_EXCL a. If this succeeds, write perl-flock\n b. If this fails, i. open the file for read+write, without create; if this fails ENOENT go back to start ii. check whether the file contains only perl-flock\n and if not sleep 1 and go back to start (If we weren't trying to be compatible with an old scheme we could just open the file as in 1(b)(i) and not write anything to it.) 2. Lock .lock with Perl's flock 3. fstat your fd on .lock and stat .lock; compare dev and inum if not identical then race has happened, go back to start -- lock is now held -- 4. Read old file contents } standard, nearly 5. Write new file contents to temporary file } only, safe method 6. Unlink old backup file } of updating a file 7. link(2) old file to backup file}on unix 8. rename(2) temporary file to primary file } -- release lock: -- 9. unlink .lock } must be done 10. close our fd on .lock } in this order Ian. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#2531: PATCH: Make install-info use fcntl (via perl's flock) for locking
tags 2531 +patch
thanks
This patch converts install-info to use perl's flock for locking. I have been
told that this uses the system's fcntl in Debian's perl installation. (It is
written to tolerate any implementation of flock which perl may supply, however.)
As part of this change, it now operates directly on the dir file rather than
creating a new dir file and then relocating it. (This is the cleanest way to
make the locking actually work right.) Accordingly the creation of the backup
dir file is moved *before* the actual work rather than after it.
This has been tested (by hand-invocation). It works.
It *might* require that dpkg acquire a dependency on a version of perl-base
newer
than something or other. I'm not sure because I'm not sure when exactly the
necessary functionality arrived in perl. I doubt that it requires such a
dependency; I believe all the necessary functionality is present in 5.8.4
(which
is in 'stable'); I'm just not 100% sure.
This is copyright-worthy content, so here's my copyright notice for
debian/copyright
(should you choose to apply this patch):
Copyright 2006 Nathanael Nerode [EMAIL PROTECTED]
--- install-info.pl.orig2006-06-07 00:58:26.0 -0400
+++ install-info.pl 2006-06-07 02:27:38.0 -0400
@@ -1,6 +1,8 @@
#!/usr/bin/perl --
use Text::Wrap;
+use Fcntl ':flock';
+use Fcntl ':seek';
my $dpkglibdir = .; # This line modified by Makefile
push (@INC, $dpkglibdir);
@@ -301,18 +303,28 @@
}
}
-if (!$nowrite !link($dirfile, $dirfile.lock)) {
-printf( STDERR _g(%s: failed to lock dir for editing! %s).\n,
- $name, $! );
-printf( STDERR _g(try deleting %s?).\n, $dirfile.lock)
- if $!{EEXIST};
+# Handle (sort of) being run concurrently with older versions.
+if (!$nowrite -e $dirfile.lock) {
+printf( STDERR _g(%s: old lockfile still present! ).\n,
+ $name);
+printf( STDERR _g(try deleting %s?).\n, $dirfile.lock);
exit 1;
}
-open(OLD,$dirfile) || ulquit(sprintf(_g(open %s: %s), $dirfile, $!));
[EMAIL PROTECTED] OLD;
-eof(OLD) || ulquit(sprintf(_g(read %s: %s), $dirfile, $!));
-close(OLD) || ulquit(sprintf(_g(close %s after read: %s), $dirfile, $!));
+if (!$nowrite) {
+# Open for reading and writing, and lock it.
+open(DIRFILE,+,$dirfile) || ulquit(sprintf(_g(open %s: %s),
$dirfile, $!));
+flock(DIRFILE, LOCK_EX);
+# Back it up. Since we'll be erasing the original, this is crucial.
+unlink($dirfile.old);
+system ('cp', $dirfile, $dirfile.old)
+ ulquit(sprintf(_g(cannot backup old %s, giving up: %s), $dirfile,
$!));
+} else {
+open(DIRFILE,,$dirfile) || ulquit(sprintf(_g(open %s: %s), $dirfile,
$!));
+}
+
[EMAIL PROTECTED] DIRFILE;
+eof(DIRFILE) || ulquit(sprintf(_g(read %s: %s), $dirfile, $!));
while (($#work = 0) ($work[$#work] !~ m/\S/)) { $#work--; }
@@ -478,27 +490,18 @@
}
if (!$nowrite) {
-open(NEW, $dirfile.new) || ulquit(sprintf(_g(create %s: %s),
$dirfile.new, $!));
-print(NEW @head,join(\n,@newwork)) ||
- ulquit(sprintf(_g(write %s: %s), $dirfile.new, $!));
-close(NEW) || ulquit(sprintf(_g(close %s: %s), $dirfile.new, $!));
-
-unlink($dirfile.old);
-link($dirfile, $dirfile.old) ||
- ulquit(sprintf(_g(cannot backup old %s, giving up: %s), $dirfile,
$!));
-rename($dirfile.new, $dirfile) ||
- ulquit(sprintf(_g(install new %s: %s), $dirfile, $!));
-
-unlink($dirfile.lock) ||
- die sprintf(_g(%s: unlock %s: %s), $name, $dirfile, $!).\n;
-system ('cp', $dirfile, $backup)
- warn sprintf(_g(%s: couldn't backup %s in %s: %s), $name, $dirfile,
$backup, $!).\n;
+# Switch from reading to writing, still holding the same lock on the same
file
+seek(DIRFILE,0,SEEK_SET)
+ || ulquit(sprintf(_g(seeking start of %s: %s), $dirfile, $!));
+truncate(DIRFILE, 0)
+ || ulquit(sprintf(_g(truncating %s: %s), $dirfile, $!));
+print(DIRFILE @head,join(\n,@newwork)) ||
+ ulquit(sprintf(_g(writing new %s: %s), $dirfile, $!));
+flock(DIRFILE, LOCK_UN);
}
+close(DIRFILE) || ulquit(sprintf(_g(close %s: %s), $dirfile, $!));
sub ulquit {
-unlink($dirfile.lock) ||
- warn sprintf(_g(%s: warning - unable to unlock %s: %s),
-$name, $dirfile, $!).\n;
die $name: $_[0]\n;
}
--
Nathanael Nerode [EMAIL PROTECTED]
[Insert famous quote here]
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
