Bug#538194: fdm doesn't handle EXDEV when linking files.
This is committed to CVS HEAD now and will be in the next release of fdm when I get around to it. Thanks! On Sat, Jul 25, 2009 at 08:02:46PM +0200, opter_l wrote: As discussed here and on IRC. I have replaced the link() call in deliver-maildir.c by a call to a new `safemove()' function. This function move a file with link() + unlink() or if it fails on EXDEV by using rename(). The patch is joined with this mail. I have tested it on AFS and ext3; the regressions test suite successfully passed on OpenBSD 4.5. Nicholas, I let you write the bits in the user manual about using fdm with this patch on AFS. I believe that it will be more clear if you do it. Thanks a lot for your help about this. I am open to any suggestion. -- Louis Opter -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#538194: fdm doesn't handle EXDEV when linking files.
I'm going to run with this for a few days and if I don't see any problems I'll commit it, then it'll be in 1.7 when I get around to doing some more stuff on fdm. Thanks On Sat, Jul 25, 2009 at 08:02:46PM +0200, opter_l wrote: As discussed here and on IRC. I have replaced the link() call in deliver-maildir.c by a call to a new `safemove()' function. This function move a file with link() + unlink() or if it fails on EXDEV by using rename(). The patch is joined with this mail. I have tested it on AFS and ext3; the regressions test suite successfully passed on OpenBSD 4.5. Nicholas, I let you write the bits in the user manual about using fdm with this patch on AFS. I believe that it will be more clear if you do it. Thanks a lot for your help about this. I am open to any suggestion. -- Louis Opter -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#538194: fdm doesn't handle EXDEV when linking files.
Sorry for the delay, I was a bit busy yesterday. This looks great to me, I will give it further test, add the manual bits and commit it later on today or tomorrow. Thanks for your work! On Sat, Jul 25, 2009 at 08:02:46PM +0200, opter_l wrote: As discussed here and on IRC. I have replaced the link() call in deliver-maildir.c by a call to a new `safemove()' function. This function move a file with link() + unlink() or if it fails on EXDEV by using rename(). The patch is joined with this mail. I have tested it on AFS and ext3; the regressions test suite successfully passed on OpenBSD 4.5. Nicholas, I let you write the bits in the user manual about using fdm with this patch on AFS. I believe that it will be more clear if you do it. Thanks a lot for your help about this. I am open to any suggestion. -- Louis Opter -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#538194: fdm doesn't handle EXDEV when linking files.
As discussed here and on IRC.
I have replaced the link() call in deliver-maildir.c by a call to a new
`safemove()' function.
This function move a file with link() + unlink() or if it fails on EXDEV by
using rename().
The patch is joined with this mail. I have tested it on AFS and ext3; the
regressions test suite successfully passed on OpenBSD 4.5.
Nicholas, I let you write the bits in the user manual about using fdm with
this
patch on AFS. I believe that it will be more clear if you do it.
Thanks a lot for your help about this.
I am open to any suggestion.
--
Louis Opterdiff -u fdm-1.5.orig/deliver-maildir.c fdm-1.5-3.debian.louis/deliver-maildir.c
--- fdm-1.5.orig/deliver-maildir.c 2008-03-06 10:25:32.0 +0100
+++ fdm-1.5-3.debian.louis/deliver-maildir.c 2009-07-25 16:30:52.127297406 +0200
@@ -190,7 +190,7 @@
log_debug2(%s: writing to %s, a-name, src);
n = write(fd, m-data, m-size);
if (n 0 || (size_t) n != m-size || fsync(fd) != 0)
- goto error_unlink;
+ goto error_cleanup;
close(fd);
fd = -1;
@@ -199,24 +199,18 @@
* back to find another name in the tmp directory.
*/
if (ppath(dst, sizeof dst, %s/new/%s, path, name) != 0)
- goto error_unlink;
+ goto error_cleanup;
log_debug2(
- %s: linking .../tmp/%s to .../new/%s, a-name, name, name);
- if (link(src, dst) != 0) {
+ %s: moving .../tmp/%s to .../new/%s, a-name, name, name);
+ if (safemove(src, dst) != 0) {
if (errno == EEXIST) {
- log_debug2(%s: %s: link failed, a-name, src);
- if (unlink(src) != 0)
-fatal(unlink failed);
+ log_debug2(%s: %s: moving failed, a-name, src);
cleanup_deregister(src);
goto restart;
}
- goto error_unlink;
+ goto error_cleanup;
}
- /* Unlink the original tmp file. */
- log_debug2(%s: unlinking .../tmp/%s, a-name, name);
- if (unlink(src) != 0)
- goto error_unlink;
cleanup_deregister(src);
/* Save the mail file as a tag. */
@@ -226,9 +220,7 @@
xfree(path);
return (DELIVER_SUCCESS);
-error_unlink:
- if (unlink(src) != 0)
- fatal(unlink failed);
+error_cleanup:
cleanup_deregister(src);
error_log:
diff -u fdm-1.5.orig/fdm.h fdm-1.5-3.debian.louis/fdm.h
--- fdm-1.5.orig/fdm.h 2008-03-06 10:25:32.0 +0100
+++ fdm-1.5-3.debian.louis/fdm.h 2009-07-25 16:37:12.643309617 +0200
@@ -844,6 +844,7 @@
const char *checkmode(struct stat *, mode_t);
const char *checkowner(struct stat *, uid_t);
const char *checkgroup(struct stat *, gid_t);
+int safemove(const char *, const char *);
/* mail.c */
int mail_open(struct mail *, size_t);
diff -u fdm-1.5.orig/file.c fdm-1.5-3.debian.louis/file.c
--- fdm-1.5.orig/file.c 2008-03-06 10:25:32.0 +0100
+++ fdm-1.5-3.debian.louis/file.c 2009-07-25 17:09:36.353255268 +0200
@@ -314,3 +314,39 @@
bad group: %lu, should be %lu, (u_long) sb-st_gid, (u_long) gid);
return (msg);
}
+
+/*
+ * Move file oldpath to newpath. oldpath is always removed even in case of
+ * failure.
+ *
+ * It returns 0 on success and -1 on failure with errno set.
+ *
+ * This function use link + unlink or stat + rename if link fail with EXDEV.
+ * (see http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=538194)
+ */
+int
+safemove(const char *oldpath, const char *newpath)
+{
+ int ret;
+ int errsave;
+ struct stat sb;
+
+ ret = link(oldpath, newpath);
+ if (ret != 0 errno == EXDEV) {
+ ret = stat(newpath, sb);
+ if (ret == -1) {
+ if (errno == ENOENT)
+ret = rename(oldpath, newpath);
+ } else {
+ ret = -1;
+ errno = EEXIST;
+ }
+ }
+
+ errsave = errno;
+ if (unlink(oldpath) != 0 errno != ENOENT)
+ fatal(unlink failed);
+ errno = errsave;
+
+ return (ret);
+}
Bug#538194: fdm doesn't handle EXDEV when linking files.
On Thu, 23 Jul 2009 23:38:08 +0100, Nicholas Marriott nicholas.marri...@gmail.com wrote: Hi Thanks for the report. This isn't really an fdm bug per se, the point of maildirs is to allow delivery to happen safely without locking. On AFS you are probably best using mboxes which have flaws but can be locked atomically. Saying that, I don't see a reason not to make this work for people who are prepared to accept the risk or who know for sure they are delivering mail sequentially. With fdm, you will want to set queue-high/queue-low to 1, and perhaps parallel-accounts to 1 if you have multiple accounts delivering to the same maildir. You mean because of locks in the file system code with rename ? I had not thought of. But, the read on a maildir -with a big amount of mails- stays faster, isn't it ? (Actually, I am new to fdm/maildir/mboxes/mutt... I used Evolution and webmails...) This diff is reasonable, but if possible I'd rather it was implemented as a wrapper function (xlink?) which has the same semantics as link but falls back to stat/rename if it fails with EXDEV. Okay, I will try to study it, tomorrow. It will also probably need a note in the manual documenting this behaviour. Of course. Offhand I don't think there are any other places where fdm does a cross-directory link. Ok, I only did a quick grep. Best Regards. -- Louis Opter -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#538194: fdm doesn't handle EXDEV when linking files.
Hi Saying that, I don't see a reason not to make this work for people who are prepared to accept the risk or who know for sure they are delivering mail sequentially. With fdm, you will want to set queue-high/queue-low to 1, and perhaps parallel-accounts to 1 if you have multiple accounts delivering to the same maildir. You mean because of locks in the file system code with rename ? I meant there is a possibility of having races between fdm delivery children (it forks a separate one for each child), although I had another look and since the filename contains the PID that is more unlikely than I first thought - most platforms don't reuse PIDs that quickly. It is still unpredictable so do note that if 1 process is accessing the maildir at a time, you could lose mail, even if it is an outside chance. I had not thought of. But, the read on a maildir -with a big amount of mails- stays faster, isn't it ? (Actually, I am new to fdm/maildir/mboxes/mutt... I used Evolution and webmails...) It depends how it is implemented but there is no reason mbox reads or appends should be slower than maildir; a full mailbox scan will be slower and deletion will be much slower. The biggest problems with mboxes is that only one process can access them simultaneously and some big operations (like delete) can require a copy and rewrite of the entire file to do safely. I really don't like mboxes, I just wanted to note that they do have locking so you will not lose mail due to this limitation in AFS. That doesn't excuse their other problems though. Thinking about it again if I was you I would probably use maildirs anyway (once fdm is changed to work around the problem) but you should try to be careful that as much as possible only one program writes to the maildir at a time. This diff is reasonable, but if possible I'd rather it was implemented as a wrapper function (xlink?) which has the same semantics as link but falls back to stat/rename if it fails with EXDEV. Okay, I will try to study it, tomorrow. Great! Thanks Nicholas -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#538194: fdm doesn't handle EXDEV when linking files.
Package: fdm
Version: 1.5-3
Severity: important
Tags: patch
Dear maintainer,
link() fails with EXDEV when you try to link files accross file systems.
On the AFS file system, this also occurs between directories :
http://docs.openafs.org/AdminGuide/ch02.html#HDRWQ32
This special case breaks fdm when using maildirs on OpenAFS.
I suggest a patch which workaround EXDEV by using stat() and rename().
I haven't run fdm's regressions tests on it. I have just checked it on
AFS and ext3.
A similar issue has been fixed in openssh (see #352589).
The same bug certainly exists in other parts of fdm.
Here is the code :
--- fdm-1.5.orig/deliver-maildir.c 2008-03-06 10:25:32.0 +0100
+++ fdm-1.5-3.debian.louis/deliver-maildir.c2009-07-23 21:28:34.418851829
+0200
@@ -146,6 +146,7 @@
char src[MAXPATHLEN], dst[MAXPATHLEN];
int fd;
ssize_t n;
+ struct stat sb;
name = NULL;
fd = -1;
@@ -195,28 +196,52 @@
fd = -1;
/*
-* Create the new path and attempt to link it. A failed link jumps
-* back to find another name in the tmp directory.
+* Create the new path and attempt to link it. A failed link on EEXIST
+* jumps back to find another name in the tmp directory.
*/
if (ppath(dst, sizeof dst, %s/new/%s, path, name) != 0)
goto error_unlink;
log_debug2(
%s: linking .../tmp/%s to .../new/%s, a-name, name, name);
if (link(src, dst) != 0) {
- if (errno == EEXIST) {
- log_debug2(%s: %s: link failed, a-name, src);
+ log_debug2(%s: %s: link failed, a-name, src);
+ /*
+* EXDEV must also be handled, especially for the AFS filesystem
+* where hardlinks can't traverse directories :
+* http://docs.openafs.org/AdminGuide/ch02.html#HDRWQ32
+*/
+ if (errno == EXDEV) {
+ log_debug2(%s: renaming .../tmp/%s to .../new/%s,
+ a-name, name, name);
+ /*
+* But stat + rename can be racy.
+*/
+ if (stat(dst, sb) == -1) {
+ if (errno != ENOENT || rename(src, dst) != 0) {
+ log_debug2(%s: %s: rename failed,
+ a-name, src);
+ goto error_cleanup;
+ }
+ } else { /* EEXIST */
+ if (unlink(src) != 0)
+ fatal(unlink failed);
+ cleanup_deregister(src);
+ goto restart;
+ }
+ } else if (errno == EEXIST) {
if (unlink(src) != 0)
fatal(unlink failed);
cleanup_deregister(src);
goto restart;
- }
- goto error_unlink;
+ } else /* Dot it only if we are not on EXDEV */
+ goto error_unlink;
+ } else {
+ /* Unlink the original tmp file. */
+ log_debug2(%s: unlinking .../tmp/%s, a-name, name);
+ if (unlink(src) != 0)
+ goto error_unlink;
}
- /* Unlink the original tmp file. */
- log_debug2(%s: unlinking .../tmp/%s, a-name, name);
- if (unlink(src) != 0)
- goto error_unlink;
cleanup_deregister(src);
/* Save the mail file as a tag. */
@@ -229,6 +254,7 @@
error_unlink:
if (unlink(src) != 0)
fatal(unlink failed);
+error_cleanup:
cleanup_deregister(src);
error_log:
Please, tell me any suggestion.
Best regards, Louis Opter.
-- System Information:
Debian Release: 5.0.2
APT prefers stable
APT policy: (500, 'stable')
Architecture: amd64 (x86_64)
Kernel: Linux 2.6.26-2-amd64 (SMP w/2 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/bash
Versions of packages fdm depends on:
ii adduser 3.110 add and remove users and groups
ii libc62.7-18 GNU C Library: Shared libraries
ii libpcre3 7.6-2.1 Perl 5 Compatible Regular Expressi
ii libssl0.9.8 0.9.8g-15+lenny1SSL shared libraries
ii libtdb1 1.1.2~git20080615-1 Trivial Database - shared library
ii zlib1g 1:1.2.3.3.dfsg-12 compression library - runtime
fdm recommends no packages.
fdm suggests no packages.
-- no debconf information
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
Bug#538194: fdm doesn't handle EXDEV when linking files.
[ Ccing the fdm-users mailing list. Please reply to all addresses in ]
[ To: and Cc: in order to keep all involved parties and the debian ]
[ BTS in the loop. ]
A new bug report via the debian BTS, which includes a suggestion for a
fix.
The referenced bug #352589 can be found here:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=352589
Regards, Frank
Louis Opter opte...@epitech.net:
Package: fdm
Version: 1.5-3
Severity: important
Tags: patch
Dear maintainer,
link() fails with EXDEV when you try to link files accross file systems.
On the AFS file system, this also occurs between directories :
http://docs.openafs.org/AdminGuide/ch02.html#HDRWQ32
This special case breaks fdm when using maildirs on OpenAFS.
I suggest a patch which workaround EXDEV by using stat() and rename().
I haven't run fdm's regressions tests on it. I have just checked it on
AFS and ext3.
A similar issue has been fixed in openssh (see #352589).
The same bug certainly exists in other parts of fdm.
Here is the code :
--- fdm-1.5.orig/deliver-maildir.c 2008-03-06 10:25:32.0 +0100
+++ fdm-1.5-3.debian.louis/deliver-maildir.c2009-07-23 21:28:34.418851829
+0200
@@ -146,6 +146,7 @@
char src[MAXPATHLEN], dst[MAXPATHLEN];
int fd;
ssize_t n;
+ struct stat sb;
name = NULL;
fd = -1;
@@ -195,28 +196,52 @@
fd = -1;
/*
-* Create the new path and attempt to link it. A failed link jumps
-* back to find another name in the tmp directory.
+* Create the new path and attempt to link it. A failed link on EEXIST
+* jumps back to find another name in the tmp directory.
*/
if (ppath(dst, sizeof dst, %s/new/%s, path, name) != 0)
goto error_unlink;
log_debug2(
%s: linking .../tmp/%s to .../new/%s, a-name, name, name);
if (link(src, dst) != 0) {
- if (errno == EEXIST) {
- log_debug2(%s: %s: link failed, a-name, src);
+ log_debug2(%s: %s: link failed, a-name, src);
+ /*
+* EXDEV must also be handled, especially for the AFS filesystem
+* where hardlinks can't traverse directories :
+* http://docs.openafs.org/AdminGuide/ch02.html#HDRWQ32
+*/
+ if (errno == EXDEV) {
+ log_debug2(%s: renaming .../tmp/%s to .../new/%s,
+ a-name, name, name);
+ /*
+* But stat + rename can be racy.
+*/
+ if (stat(dst, sb) == -1) {
+ if (errno != ENOENT || rename(src, dst) != 0) {
+ log_debug2(%s: %s: rename failed,
+ a-name, src);
+ goto error_cleanup;
+ }
+ } else { /* EEXIST */
+ if (unlink(src) != 0)
+ fatal(unlink failed);
+ cleanup_deregister(src);
+ goto restart;
+ }
+ } else if (errno == EEXIST) {
if (unlink(src) != 0)
fatal(unlink failed);
cleanup_deregister(src);
goto restart;
- }
- goto error_unlink;
+ } else /* Dot it only if we are not on EXDEV */
+ goto error_unlink;
+ } else {
+ /* Unlink the original tmp file. */
+ log_debug2(%s: unlinking .../tmp/%s, a-name, name);
+ if (unlink(src) != 0)
+ goto error_unlink;
}
- /* Unlink the original tmp file. */
- log_debug2(%s: unlinking .../tmp/%s, a-name, name);
- if (unlink(src) != 0)
- goto error_unlink;
cleanup_deregister(src);
/* Save the mail file as a tag. */
@@ -229,6 +254,7 @@
error_unlink:
if (unlink(src) != 0)
fatal(unlink failed);
+error_cleanup:
cleanup_deregister(src);
error_log:
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#538194: fdm doesn't handle EXDEV when linking files.
Hi
Thanks for the report.
This isn't really an fdm bug per se, the point of maildirs is to allow delivery
to happen safely without locking. On AFS you are probably best using mboxes
which have flaws but can be locked atomically.
Saying that, I don't see a reason not to make this work for people who are
prepared to accept the risk or who know for sure they are delivering mail
sequentially. With fdm, you will want to set queue-high/queue-low to 1, and
perhaps parallel-accounts to 1 if you have multiple accounts delivering to the
same maildir.
This diff is reasonable, but if possible I'd rather it was implemented as a
wrapper function (xlink?) which has the same semantics as link but falls back
to stat/rename if it fails with EXDEV.
It will also probably need a note in the manual documenting this behaviour.
Offhand I don't think there are any other places where fdm does a
cross-directory link.
Regards
Nicholas
On Thu, Jul 23, 2009 at 11:50:37PM +0200, Frank Terbeck wrote:
[ Ccing the fdm-users mailing list. Please reply to all addresses in ]
[ To: and Cc: in order to keep all involved parties and the debian ]
[ BTS in the loop. ]
A new bug report via the debian BTS, which includes a suggestion for a
fix.
The referenced bug #352589 can be found here:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=352589
Regards, Frank
Louis Opter opte...@epitech.net:
Package: fdm
Version: 1.5-3
Severity: important
Tags: patch
Dear maintainer,
link() fails with EXDEV when you try to link files accross file systems.
On the AFS file system, this also occurs between directories :
http://docs.openafs.org/AdminGuide/ch02.html#HDRWQ32
This special case breaks fdm when using maildirs on OpenAFS.
I suggest a patch which workaround EXDEV by using stat() and rename().
I haven't run fdm's regressions tests on it. I have just checked it on
AFS and ext3.
A similar issue has been fixed in openssh (see #352589).
The same bug certainly exists in other parts of fdm.
Here is the code :
--- fdm-1.5.orig/deliver-maildir.c2008-03-06 10:25:32.0 +0100
+++ fdm-1.5-3.debian.louis/deliver-maildir.c 2009-07-23 21:28:34.418851829
+0200
@@ -146,6 +146,7 @@
char src[MAXPATHLEN], dst[MAXPATHLEN];
int fd;
ssize_t n;
+ struct stat sb;
name = NULL;
fd = -1;
@@ -195,28 +196,52 @@
fd = -1;
/*
- * Create the new path and attempt to link it. A failed link jumps
- * back to find another name in the tmp directory.
+ * Create the new path and attempt to link it. A failed link on EEXIST
+ * jumps back to find another name in the tmp directory.
*/
if (ppath(dst, sizeof dst, %s/new/%s, path, name) != 0)
goto error_unlink;
log_debug2(
%s: linking .../tmp/%s to .../new/%s, a-name, name, name);
if (link(src, dst) != 0) {
- if (errno == EEXIST) {
- log_debug2(%s: %s: link failed, a-name, src);
+ log_debug2(%s: %s: link failed, a-name, src);
+ /*
+ * EXDEV must also be handled, especially for the AFS filesystem
+ * where hardlinks can't traverse directories :
+ * http://docs.openafs.org/AdminGuide/ch02.html#HDRWQ32
+ */
+ if (errno == EXDEV) {
+ log_debug2(%s: renaming .../tmp/%s to .../new/%s,
+ a-name, name, name);
+ /*
+ * But stat + rename can be racy.
+ */
+ if (stat(dst, sb) == -1) {
+ if (errno != ENOENT || rename(src, dst) != 0) {
+ log_debug2(%s: %s: rename failed,
+ a-name, src);
+ goto error_cleanup;
+ }
+ } else { /* EEXIST */
+ if (unlink(src) != 0)
+ fatal(unlink failed);
+ cleanup_deregister(src);
+ goto restart;
+ }
+ } else if (errno == EEXIST) {
if (unlink(src) != 0)
fatal(unlink failed);
cleanup_deregister(src);
goto restart;
- }
- goto error_unlink;
+ } else /* Dot it only if we are not on EXDEV */
+ goto error_unlink;
+ } else {
+ /* Unlink the original tmp file. */
+ log_debug2(%s: unlinking .../tmp/%s, a-name, name);
+ if (unlink(src) != 0)
+
