From 3e9b4107fddb2aeb52c144add3f09ab019551bd2 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Sun, 23 Jul 2023 12:15:29 +0200 Subject: [PATCH] move "link or copy" functionality to moxio and add a bit more logging for unexpected failures when closing files. and make tests pass with a TMPDIR on a different filesystem than the testdata directory. --- backup.go | 12 ++++--- http/account.go | 2 +- http/admin.go | 6 ++-- imapserver/server.go | 57 +++++++-------------------------- moxio/linkcopy.go | 71 ++++++++++++++++++++++++++++++++++++++++++ moxio/linkcopy_test.go | 57 +++++++++++++++++++++++++++++++++ queue/queue.go | 34 ++------------------ queue/queue_test.go | 14 --------- store/account.go | 30 ++---------------- store/account_test.go | 14 --------- store/export_test.go | 2 +- store/tmp.go | 10 +++--- 12 files changed, 165 insertions(+), 144 deletions(-) create mode 100644 moxio/linkcopy.go create mode 100644 moxio/linkcopy_test.go diff --git a/backup.go b/backup.go index d3e7c20..92200e7 100644 --- a/backup.go +++ b/backup.go @@ -240,15 +240,19 @@ func backupctl(ctx context.Context, ctl *ctl) { if err != nil { return false, fmt.Errorf("open source path %s: %v", srcpath, err) } - defer sf.Close() + defer func() { + err := sf.Close() + ctl.log.Check(err, "closing copied source file") + }() df, err := os.OpenFile(dstpath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0660) if err != nil { - return false, fmt.Errorf("open destination path %s: %v", dstpath, err) + return false, fmt.Errorf("create destination path %s: %v", dstpath, err) } defer func() { if df != nil { - df.Close() + err := df.Close() + ctl.log.Check(err, "closing partial destination file") } }() if _, err := io.Copy(df, sf); err != nil { @@ -257,7 +261,7 @@ func backupctl(ctx context.Context, ctl *ctl) { err = df.Close() df = nil if err != nil { - return false, fmt.Errorf("close: %v", err) + return false, fmt.Errorf("closing destination file: %v", err) } return false, nil } diff --git a/http/account.go b/http/account.go index 251f219..8d6e81d 100644 --- a/http/account.go +++ b/http/account.go @@ -35,7 +35,7 @@ var accountapiJSON []byte //go:embed account.html var accountHTML []byte -var accountDoc = mustParseAPI(accountapiJSON) +var accountDoc = mustParseAPI("account", accountapiJSON) var accountSherpaHandler http.Handler diff --git a/http/admin.go b/http/admin.go index 6bb5638..ab3a4d4 100644 --- a/http/admin.go +++ b/http/admin.go @@ -59,14 +59,14 @@ var adminapiJSON []byte //go:embed admin.html var adminHTML []byte -var adminDoc = mustParseAPI(adminapiJSON) +var adminDoc = mustParseAPI("admin", adminapiJSON) var adminSherpaHandler http.Handler -func mustParseAPI(buf []byte) (doc sherpadoc.Section) { +func mustParseAPI(api string, buf []byte) (doc sherpadoc.Section) { err := json.Unmarshal(buf, &doc) if err != nil { - xlog.Fatalx("parsing api docs", err) + xlog.Fatalx("parsing api docs", err, mlog.Field("api", api)) } return doc } diff --git a/imapserver/server.go b/imapserver/server.go index 443e653..312fec3 100644 --- a/imapserver/server.go +++ b/imapserver/server.go @@ -3065,15 +3065,25 @@ func (c *conn) cmdxCopy(isUID bool, tag, cmd string, p *parser) { } // Copy message files to new message ID's. + syncDirs := map[string]struct{}{} for i := range origMsgIDs { src := c.account.MessagePath(origMsgIDs[i]) dst := c.account.MessagePath(newMsgIDs[i]) - os.MkdirAll(filepath.Dir(dst), 0770) // todo optimization: keep track of dirs we already created, don't create them again - err := c.linkOrCopyFile(dst, src) + dstdir := filepath.Dir(dst) + if _, ok := syncDirs[dstdir]; !ok { + os.MkdirAll(dstdir, 0770) + syncDirs[dstdir] = struct{}{} + } + err := moxio.LinkOrCopy(c.log, dst, src, nil, true) xcheckf(err, "link or copy file %q to %q", src, dst) createdIDs = append(createdIDs, newMsgIDs[i]) } + for dir := range syncDirs { + err := moxio.SyncDir(dir) + xcheckf(err, "sync directory") + } + err = c.account.RetrainMessages(context.TODO(), c.log, tx, nmsgs, false) xcheckf(err, "train copied messages") }) @@ -3095,49 +3105,6 @@ func (c *conn) cmdxCopy(isUID bool, tag, cmd string, p *parser) { c.writeresultf("%s OK [COPYUID %d %s %s] copied", tag, mbDst.UIDValidity, compactUIDSet(origUIDs).String(), compactUIDSet(newUIDs).String()) } -func (c *conn) linkOrCopyFile(dst, src string) error { - // Try hardlink first. - if err := os.Link(src, dst); err == nil { - return nil - } - - // File system may not support hardlinks, or link would cross file systems. Do a regular file copy. - sf, err := os.Open(src) - if err != nil { - return err - } - defer func() { - err := sf.Close() - c.xsanity(err, "closing copied src file") - }() - - df, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0660) - if err != nil { - return err - } - defer func() { - if df != nil { - err = os.Remove(df.Name()) - c.xsanity(err, "removing unfinished dst file") - err = df.Close() - c.xsanity(err, "closing unfinished dst file") - } - }() - - if _, err := io.Copy(df, sf); err != nil { - return err - } - if err := df.Close(); err != nil { - xerr := os.Remove(df.Name()) - c.xsanity(xerr, "removing unfinished dst file") - df = nil - return err - } - // todo: may need to do a file/dir sync to flush to disk. better to do it once after multiple linkOrCopyFile calls. - df = nil - return nil -} - // Move moves messages from the currently selected/active mailbox to a named mailbox. // // State: Selected diff --git a/moxio/linkcopy.go b/moxio/linkcopy.go new file mode 100644 index 0000000..24ff5a3 --- /dev/null +++ b/moxio/linkcopy.go @@ -0,0 +1,71 @@ +package moxio + +import ( + "fmt" + "io" + "os" + + "github.com/mjl-/mox/mlog" +) + +// LinkOrCopy attempts to make a hardlink dst. If that fails, it will try to do a +// regular file copy. If srcReaderOpt is not nil, it will be used for reading. If +// sync is true and the file is copied, Sync is called on the file after writing to +// ensure the file is written on disk. Callers should also sync the directory of +// the destination file, but may want to do that after linking/copying multiple +// files. If dst was created and an error occurred, it is removed. +func LinkOrCopy(log *mlog.Log, dst, src string, srcReaderOpt io.Reader, sync bool) (rerr error) { + // Try hardlink first. + err := os.Link(src, dst) + if err == nil { + return nil + } else if os.IsNotExist(err) { + // No point in trying with regular copy, we would fail again. Either src doesn't + // exist or dst directory doesn't exist. + return err + } + + // File system may not support hardlinks, or link could be crossing file systems. + // Do a regular file copy. + if srcReaderOpt == nil { + sf, err := os.Open(src) + if err != nil { + return fmt.Errorf("open source file: %w", err) + } + defer func() { + err := sf.Close() + log.Check(err, "closing copied source file") + }() + srcReaderOpt = sf + } + + df, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0660) + if err != nil { + return fmt.Errorf("create destination: %w", err) + } + defer func() { + if df != nil { + err = os.Remove(dst) + log.Check(err, "removing partial destination file") + err = df.Close() + log.Check(err, "closing partial destination file") + } + }() + + if _, err := io.Copy(df, srcReaderOpt); err != nil { + return fmt.Errorf("copy: %w", err) + } + if sync { + if err := df.Sync(); err != nil { + return fmt.Errorf("sync destination: %w", err) + } + } + err = df.Close() + df = nil + if err != nil { + err := os.Remove(dst) + log.Check(err, "removing partial destination file") + return err + } + return nil +} diff --git a/moxio/linkcopy_test.go b/moxio/linkcopy_test.go new file mode 100644 index 0000000..026a290 --- /dev/null +++ b/moxio/linkcopy_test.go @@ -0,0 +1,57 @@ +package moxio + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/mjl-/mox/mlog" +) + +func tcheckf(t *testing.T, err error, format string, args ...any) { + if err != nil { + t.Helper() + t.Fatalf("%s: %s", fmt.Sprintf(format, args...), err) + } +} + +func TestLinkOrCopy(t *testing.T) { + log := mlog.New("linkorcopy") + + // link in same directory. file exists error. link to file in non-existent + // directory (exists error). link to file in system temp dir (hopefully other file + // system). + src := "linkorcopytest-src.txt" + f, err := os.Create(src) + tcheckf(t, err, "creating test file") + defer os.Remove(src) + err = LinkOrCopy(log, "linkorcopytest-dst.txt", src, nil, false) + tcheckf(t, err, "linking file") + err = os.Remove("linkorcopytest-dst.txt") + tcheckf(t, err, "remove dst") + + err = LinkOrCopy(log, "bogus/linkorcopytest-dst.txt", src, nil, false) + if err == nil || !os.IsNotExist(err) { + t.Fatalf("expected is not exist, got %v", err) + } + + // Try with copying the file. This can currently only really happen on systems that + // don't support hardlinking. Because other code and tests already use os.Rename on + // similar files, which will fail for being cross-filesystem (and we do want + // users/admins to have the mox temp dir on the same file system as the account + // files). + dst := filepath.Join(os.TempDir(), "linkorcopytest-dst.txt") + err = LinkOrCopy(log, dst, src, nil, true) + tcheckf(t, err, "copy file") + err = os.Remove(dst) + tcheckf(t, err, "removing dst") + + // Copy based on open file. + _, err = f.Seek(0, 0) + tcheckf(t, err, "seek to start") + err = LinkOrCopy(log, dst, src, f, true) + tcheckf(t, err, "copy file from reader") + err = os.Remove(dst) + tcheckf(t, err, "removing dst") +} diff --git a/queue/queue.go b/queue/queue.go index 998702f..4af7074 100644 --- a/queue/queue.go +++ b/queue/queue.go @@ -240,14 +240,9 @@ func Add(ctx context.Context, log *mlog.Log, senderAccount string, mailFrom, rcp // Could be due to cross-filesystem rename. Users shouldn't configure their systems that way. return 0, fmt.Errorf("move message into queue dir: %w", err) } - } else if err := os.Link(msgFile.Name(), dst); err != nil { - // Assume file system does not support hardlinks. Copy it instead. - if err := writeFile(dst, &moxio.AtReader{R: msgFile}); err != nil { - return 0, fmt.Errorf("copying message to new file: %s", err) - } - } - - if err := moxio.SyncDir(dstDir); err != nil { + } else if err := moxio.LinkOrCopy(log, dst, msgFile.Name(), nil, true); err != nil { + return 0, fmt.Errorf("linking/copying message to new file: %s", err) + } else if err := moxio.SyncDir(dstDir); err != nil { return 0, fmt.Errorf("sync directory: %v", err) } @@ -261,29 +256,6 @@ func Add(ctx context.Context, log *mlog.Log, senderAccount string, mailFrom, rcp return qm.ID, nil } -// write contents of r to new file dst, for delivering a message. -func writeFile(dst string, r io.Reader) error { - df, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0660) - if err != nil { - return fmt.Errorf("create: %w", err) - } - defer func() { - if df != nil { - err := df.Close() - xlog.Check(err, "closing file after failed write") - } - }() - if _, err := io.Copy(df, r); err != nil { - return fmt.Errorf("copy: %s", err) - } else if err := df.Sync(); err != nil { - return fmt.Errorf("sync: %s", err) - } else if err := df.Close(); err != nil { - return fmt.Errorf("close: %s", err) - } - df = nil - return nil -} - func formatIPDomain(d dns.IPDomain) string { if len(d.IP) > 0 { return "[" + d.IP.String() + "]" diff --git a/queue/queue_test.go b/queue/queue_test.go index 7d576d8..c1f38d9 100644 --- a/queue/queue_test.go +++ b/queue/queue_test.go @@ -491,20 +491,6 @@ func TestQueueStart(t *testing.T) { time.Sleep(100 * time.Millisecond) // Racy... we won't get notified when work is done... } -func TestWriteFile(t *testing.T) { - name := "../testdata/queue.test" - os.Remove(name) - defer os.Remove(name) - err := writeFile(name, strings.NewReader("test")) - if err != nil { - t.Fatalf("writeFile, unexpected error %v", err) - } - buf, err := os.ReadFile(name) - if err != nil || string(buf) != "test" { - t.Fatalf("writeFile, read file, got err %v, data %q", err, buf) - } -} - func TestGatherHosts(t *testing.T) { mox.Context = ctxbg diff --git a/store/account.go b/store/account.go index 299fcb1..3012a96 100644 --- a/store/account.go +++ b/store/account.go @@ -693,13 +693,11 @@ func (a *Account) DeliverMessage(log *mlog.Log, tx *bstore.Tx, m *Message, msgFi if consumeFile { if err := os.Rename(msgFile.Name(), msgPath); err != nil { + // Could be due to cross-filesystem rename. Users shouldn't configure their systems that way. return fmt.Errorf("moving msg file to destination directory: %w", err) } - } else if err := os.Link(msgFile.Name(), msgPath); err != nil { - // Assume file system does not support hardlinks. Copy it instead. - if err := writeFile(msgPath, &moxio.AtReader{R: msgFile}); err != nil { - return fmt.Errorf("copying message to new file: %w", err) - } + } else if err := moxio.LinkOrCopy(log, msgPath, msgFile.Name(), &moxio.AtReader{R: msgFile}, true); err != nil { + return fmt.Errorf("linking/copying message to new file: %w", err) } if sync { @@ -719,28 +717,6 @@ func (a *Account) DeliverMessage(log *mlog.Log, tx *bstore.Tx, m *Message, msgFi return nil } -// write contents of r to new file dst, for delivering a message. -func writeFile(dst string, r io.Reader) error { - df, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0660) - if err != nil { - return fmt.Errorf("create: %w", err) - } - defer func() { - if df != nil { - df.Close() - } - }() - if _, err := io.Copy(df, r); err != nil { - return fmt.Errorf("copy: %s", err) - } else if err := df.Sync(); err != nil { - return fmt.Errorf("sync: %s", err) - } else if err := df.Close(); err != nil { - return fmt.Errorf("close: %s", err) - } - df = nil - return nil -} - // SetPassword saves a new password for this account. This password is used for // IMAP, SMTP (submission) sessions and the HTTP account web page. func (a *Account) SetPassword(password string) error { diff --git a/store/account_test.go b/store/account_test.go index a64a621..7f7ca84 100644 --- a/store/account_test.go +++ b/store/account_test.go @@ -231,20 +231,6 @@ func TestMailbox(t *testing.T) { } } -func TestWriteFile(t *testing.T) { - name := "../testdata/account.test" - os.Remove(name) - defer os.Remove(name) - err := writeFile(name, strings.NewReader("test")) - if err != nil { - t.Fatalf("writeFile, unexpected error %v", err) - } - buf, err := os.ReadFile(name) - if err != nil || string(buf) != "test" { - t.Fatalf("writeFile, read file, got err %v, data %q", err, buf) - } -} - func TestMessageRuleset(t *testing.T) { f, err := os.Open("/dev/null") tcheck(t, err, "open") diff --git a/store/export_test.go b/store/export_test.go index edf89b4..16da834 100644 --- a/store/export_test.go +++ b/store/export_test.go @@ -30,7 +30,7 @@ func TestExport(t *testing.T) { log := mlog.New("export") - msgFile, err := os.CreateTemp("", "mox-test-export") + msgFile, err := CreateMessageTemp("mox-test-export") tcheck(t, err, "create temp") defer os.Remove(msgFile.Name()) // To be sure. const msg = "test: test\r\n\r\ntest\r\n" diff --git a/store/tmp.go b/store/tmp.go index 89a8288..7456f21 100644 --- a/store/tmp.go +++ b/store/tmp.go @@ -6,10 +6,12 @@ import ( "github.com/mjl-/mox/mox-" ) -// CreateMessageTemp creates a temporary file for a message to be delivered. -// Caller is responsible for removing the temporary file on error, and for closing the file. -// Caller should ensure the contents of the file are synced to disk before -// attempting to deliver the message. +// CreateMessageTemp creates a temporary file, e.g. for delivery. The is created in +// subdirectory tmp of the data directory, so the file is on the same file system +// as the accounts directory, so renaming files can succeed. The caller is +// responsible for closing and possibly removing the file. The caller should ensure +// the contents of the file are synced to disk before attempting to deliver the +// message. func CreateMessageTemp(pattern string) (*os.File, error) { dir := mox.DataDirPath("tmp") os.MkdirAll(dir, 0770)