common/FileUtil.cpp | 119 ++++++++++++++++++++++++++++++----------------
common/FileUtil.hpp | 47 +++++++++++++++++-
common/JailUtil.cpp | 88 +++++++++++++++++++++++++---------
loolwsd-systemplate-setup | 12 +---
4 files changed, 195 insertions(+), 71 deletions(-)
New commits:
commit d6259d6a54e0e7873b2f4eb844c75bf0003245c6
Author: Ashod Nakashian <ashod.nakash...@collabora.co.uk>
AuthorDate: Thu Jul 2 17:54:28 2020 -0400
Commit: Ashod Nakashian <ashnak...@gmail.com>
CommitDate: Tue Jul 7 19:05:23 2020 +0200
wsd: support parallel systemplate setup
When tests are run in parallel, they will all
compete to update and set up the systemplate
directory, which has a handful of files that
need to be up-to-date. This is a source of errors.
Normally, these files are linked (hard- or soft-
link, whichever succeeds). With linking, we
only need to worry about the initial setup,
as the files will never be out-of-date from
then on. However, when linking fails, we need
to copy the files, and update them (by copying
over fresh versions of the files, if necessary)
every time a new kit is forked. Copying over
is tricky, as it's not atomic. To make it
atomic, we copy the files to the destination
directory under a temporary (random) name,
and then rename to the final name (which is
atomic, including replacing the target file,
if it exists).
No such race exists in production, where there
is (or should be) but one instance of loolwsd
(which does the initial setup) and forkit
(which updates systemplate before forking
new kit instances).
This is an issue with parallel tests only.
Change-Id: I6ba1514d00a84da7397d28efeb6378619711d52f
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/97785
Tested-by: Jenkins
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoff...@gmail.com>
Reviewed-by: Ashod Nakashian <ashnak...@gmail.com>
diff --git a/common/FileUtil.cpp b/common/FileUtil.cpp
index 560a1b659..1f31e536d 100644
--- a/common/FileUtil.cpp
+++ b/common/FileUtil.cpp
@@ -13,6 +13,7 @@
#include <dirent.h>
#include <ftw.h>
+#include <sys/time.h>
#ifdef __linux
#include <sys/vfs.h>
#elif defined IOS
@@ -85,7 +86,7 @@ namespace FileUtil
return name;
}
- void copyFileTo(const std::string &fromPath, const std::string &toPath)
+ bool copy(const std::string& fromPath, const std::string& toPath, bool
log, bool throw_on_error)
{
int from = -1, to = -1;
try {
@@ -110,7 +111,10 @@ namespace FileUtil
throw;
}
- LOG_INF("Copying " << st.st_size << " bytes from " <<
anonymizeUrl(fromPath) << " to " << anonymizeUrl(toPath));
+ // Logging may be redundant and/or noisy.
+ if (log)
+ LOG_INF("Copying " << st.st_size << " bytes from " <<
anonymizeUrl(fromPath)
+ << " to " << anonymizeUrl(toPath));
char buffer[64 * 1024];
@@ -150,15 +154,24 @@ namespace FileUtil
}
close(from);
close(to);
+ return true;
}
catch (...)
{
- LOG_SYS("Failed to copy from " << anonymizeUrl(fromPath) << " to "
<< anonymizeUrl(toPath));
+ std::ostringstream oss;
+ oss << "Failed to copy from " << anonymizeUrl(fromPath) << " to "
+ << anonymizeUrl(toPath);
+ const std::string err = oss.str();
+
+ LOG_SYS(err);
close(from);
close(to);
unlink(toPath.c_str());
- throw Poco::Exception("failed to copy");
+ if (throw_on_error)
+ throw Poco::Exception(err);
}
+
+ return false;
}
std::string getTempFilePath(const std::string& srcDir, const std::string&
srcFilename, const std::string& dstFilenamePrefix)
@@ -252,6 +265,69 @@ namespace FileUtil
return path;
}
+ bool isEmptyDirectory(const char* path)
+ {
+ DIR* dir = opendir(path);
+ if (dir == nullptr)
+ return errno != EACCES; // Assume it's not empty when EACCES.
+
+ int count = 0;
+ while (readdir(dir) && ++count < 3)
+ ;
+
+ closedir(dir);
+ return count <= 2; // Discounting . and ..
+ }
+
+ bool updateTimestamps(const std::string& filename, timespec tsAccess,
timespec tsModified)
+ {
+ // The timestamp is in seconds and microseconds.
+ timeval timestamps[2]{ { tsAccess.tv_sec, tsAccess.tv_nsec / 1000 },
+ { tsModified.tv_sec, tsModified.tv_nsec / 1000
} };
+ if (utimes(filename.c_str(), timestamps) != 0)
+ {
+ LOG_SYS("Failed to update the timestamp of [" << filename << "]");
+ return false;
+ }
+
+ return true;
+ }
+
+ bool copyAtomic(const std::string& fromPath, const std::string& toPath,
bool preserveTimestamps)
+ {
+ const std::string randFilename = toPath + Util::rng::getFilename(12);
+ if (copy(fromPath, randFilename, /*log=*/false,
/*throw_on_error=*/false))
+ {
+ if (preserveTimestamps)
+ {
+ const Stat st(fromPath);
+ updateTimestamps(randFilename, st.sb().st_atim,
st.sb().st_mtim);
+ }
+
+ // Now rename atomically, replacing any existing files with the
same name.
+ if (rename(randFilename.c_str(), toPath.c_str()) == 0)
+ return true;
+
+ LOG_SYS("Failed to copy [" << fromPath << "] -> [" << toPath
+ << "] while atomically renaming:");
+ removeFile(randFilename, false); // Cleanup.
+ }
+
+ return false;
+ }
+
+ bool linkOrCopyFile(const char* source, const char* target)
+ {
+ if (link(source, target) == -1)
+ {
+ LOG_INF("link(\"" << source << "\", \"" << target << "\") failed:
" << strerror(errno)
+ << ". Will copy.");
+ return copy(source, target, /*log=*/false,
/*throw_on_error=*/false);
+ }
+
+ return true;
+ }
+
} // namespace FileUtil
namespace
@@ -406,41 +482,6 @@ namespace FileUtil
return AnonymizeUserData ? Util::anonymize(username,
AnonymizationSalt) : username;
}
- bool isEmptyDirectory(const char* path)
- {
- DIR* dir = opendir(path);
- if (dir == nullptr)
- return errno != EACCES; // Assume it's not empty when EACCES.
-
- int count = 0;
- while (readdir(dir) && ++count < 3)
- ;
-
- closedir(dir);
- return count <= 2; // Discounting . and ..
- }
-
- bool linkOrCopyFile(const char* source, const char* target)
- {
- if (link(source, target) == -1)
- {
- LOG_INF("link(\"" << source << "\", \"" << target << "\") failed:
" << strerror(errno)
- << ". Will copy.");
- try
- {
- Poco::File(source).copyTo(target);
- }
- catch (const std::exception& exc)
- {
- LOG_ERR("Copying of [" << source << "] to [" << target
- << "] failed: " << exc.what());
- return false;
- }
- }
-
- return true;
- }
-
} // namespace FileUtil
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/common/FileUtil.hpp b/common/FileUtil.hpp
index 79342a46d..7d484d3d5 100644
--- a/common/FileUtil.hpp
+++ b/common/FileUtil.hpp
@@ -70,8 +70,25 @@ namespace FileUtil
bool isEmptyDirectory(const char* path);
inline bool isEmptyDirectory(const std::string& path) { return
isEmptyDirectory(path.c_str()); }
+ /// Update the access-time and modified-time metadata for the given file.
+ bool updateTimestamps(const std::string& filename, timespec tsAccess,
timespec tsModified);
+
+ /// Copy the source file to the target.
+ bool copy(const std::string& fromPath, const std::string& toPath, bool log,
+ bool throw_on_error);
+
+ /// Atomically copy a file and optionally preserve its timestamps.
+ /// The file is copied with a temporary name, and then atomically renamed.
+ /// NOTE: toPath must be a valid filename, not a directory.
+ /// Does not log (except errors), does not throw. Returns true on success.
+ bool copyAtomic(const std::string& fromPath, const std::string& toPath,
+ bool preserveTimestamps);
+
/// Copy a file from @fromPath to @toPath, throws on failure.
- void copyFileTo(const std::string &fromPath, const std::string &toPath);
+ inline void copyFileTo(const std::string& fromPath, const std::string&
toPath)
+ {
+ copy(fromPath, toPath, /*log=*/true, /*throw_on_error=*/true);
+ }
/// Make a temp copy of a file, and prepend it with a prefix.
std::string getTempFilePath(const std::string& srcDir, const std::string&
srcFilename,
@@ -100,7 +117,7 @@ namespace FileUtil
class Stat
{
public:
- /// Stat the given path. Symbolic links are stats when @link is true.
+ /// Stat the given path. Symbolic links are stat'ed when @link is true.
Stat(const std::string& file, bool link = false)
: _path(file)
, _res(link ? lstat(file.c_str(), &_sb) : stat(file.c_str(), &_sb))
@@ -119,9 +136,33 @@ namespace FileUtil
bool isFile() const { return S_ISREG(_sb.st_mode); }
bool isLink() const { return S_ISLNK(_sb.st_mode); }
- /// Returns true iff the path exists, regarlesss of access permission.
+ /// Returns the filesize in bytes.
+ size_t size() const { return _sb.st_size; }
+
+ /// Returns the modified time.
+ timespec modifiedTime() const { return _sb.st_mtim; }
+
+ /// Returns true iff the path exists, regardless of access permission.
bool exists() const { return good() || (_errno != ENOENT && _errno !=
ENOTDIR); }
+ /// Returns true if both files exist and have
+ /// the same size and modified timestamp.
+ bool isUpToDate(const Stat& other) const
+ {
+ if (exists() && other.exists() && !isDirectory() &&
!other.isDirectory())
+ {
+ // No need to check whether they are linked or not,
+ // since if they are, the following check will match,
+ // and if they aren't, we still need to rely on the following.
+ return (size() == other.size()
+ && modifiedTime().tv_sec == other.modifiedTime().tv_sec
+ && (modifiedTime().tv_nsec / 1000000) // Millisecond
precision.
+ == (other.modifiedTime().tv_nsec / 1000000));
+ }
+
+ return false;
+ }
+
private:
const std::string _path;
struct ::stat _sb;
diff --git a/common/JailUtil.cpp b/common/JailUtil.cpp
index a104b91b1..bc0138c0f 100644
--- a/common/JailUtil.cpp
+++ b/common/JailUtil.cpp
@@ -245,63 +245,109 @@ namespace SysTemplate
/// These must be up-to-date, as they can change during
/// the long lifetime of our process. Also, it's unlikely
/// that systemplate will get re-generated after installation.
-static const auto DynamicFilePaths = { "/etc/passwd", "/etc/group",
"/etc/host.conf",
- "/etc/hosts", "/etc/nsswitch.conf",
"/etc/resolv.conf" };
+static const auto DynamicFilePaths
+ = { "/etc/passwd", "/etc/group", "/etc/host.conf",
"/etc/hosts",
+ "/etc/nsswitch.conf", "/etc/resolv.conf", "etc/timezone",
"etc/localtime" };
-/// Copy by default for KIT_IN_PROCESS.
+/// Copy (false) by default for KIT_IN_PROCESS.
static bool LinkDynamicFiles = false;
void setupDynamicFiles(const std::string& sysTemplate)
{
- LOG_INF("Setting up dynamic files in sysTemplate.");
+ LOG_INF("Setting up systemplate dynamic files in [" << sysTemplate <<
"].");
const std::string etcSysTemplatePath = Poco::Path(sysTemplate,
"etc").toString();
- LinkDynamicFiles = true;
+ LinkDynamicFiles = true; // Prefer linking, unless it fails.
for (const auto& srcFilename : DynamicFilePaths)
{
const Poco::File srcFilePath(srcFilename);
- if (!srcFilePath.exists())
+ FileUtil::Stat srcStat(srcFilename);
+ if (!srcStat.exists())
continue;
- // Remove the file to create a symlink.
- const Poco::Path dstFilePath(sysTemplate, srcFilename);
+ const std::string dstFilename = Poco::Path(sysTemplate,
srcFilename).toString();
+ FileUtil::Stat dstStat(dstFilename);
+
+ // Is it outdated?
+ if (dstStat.isUpToDate(srcStat))
+ {
+ LOG_INF("File [" << dstFilename << "] is already up-to-date.");
+ continue;
+ }
+
if (LinkDynamicFiles)
{
- LOG_INF("Linking [" << srcFilename << "] -> [" <<
dstFilePath.toString() << "].");
- FileUtil::removeFile(dstFilePath);
+ LOG_INF("Linking [" << srcFilename << "] -> [" << dstFilename <<
"].");
// Link or copy.
- if (link(srcFilename, dstFilePath.toString().c_str()) != -1)
+ if (link(srcFilename, dstFilename.c_str()) == 0)
+ continue;
+
+ // Hard-linking failed, try symbolic linking.
+ if (symlink(srcFilename, dstFilename.c_str()) == 0)
+ continue;
+
+ const int linkerr = errno;
+
+ // With parallel tests, another test might have linked already.
+ if (Poco::File(dstFilename).exists()) // stat again.
continue;
// Failed to link a file. Disable linking and copy instead.
- LOG_WRN("Failed to link [" << srcFilename << "] -> [" <<
dstFilePath.toString() << "] ("
- << strerror(errno) << "). Will copy.");
+ LOG_WRN("Failed to link ["
+ << srcFilename << "] -> [" << dstFilename << "] (" <<
strerror(linkerr)
+ << "). Will copy and disable linking dynamic system files
in this run.");
LinkDynamicFiles = false;
}
- // Linking fails, just copy.
- LOG_INF("Copying [" << srcFilename << "] -> [" <<
dstFilePath.toString() << "].");
- srcFilePath.copyTo(etcSysTemplatePath);
+ // Linking failed, just copy.
+ LOG_INF("Copying [" << srcFilename << "] -> [" << dstFilename << "].");
+ if (!FileUtil::copyAtomic(srcFilename, dstFilename, true))
+ {
+ if (!Poco::File(dstFilename).exists()) // stat again.
+ LOG_ERR("Failed to copy [" << srcFilename << "] -> [" <<
dstFilename
+ << "], some functionality may be
missing.");
+ }
}
+
+ if (LinkDynamicFiles)
+ LOG_INF("Successfully linked the systemplate dynamic files in ["
+ << sysTemplate << "] and will not need to update them again.");
}
void updateDynamicFiles(const std::string& sysTemplate)
{
+ // If the files are linked, they are always up-to-date.
if (!LinkDynamicFiles)
{
- LOG_INF("Updating dynamic files in sysTemplate.");
+ LOG_INF("Updating systemplate dynamic files in [" << sysTemplate <<
"].");
const std::string etcSysTemplatePath = Poco::Path(sysTemplate,
"etc").toString();
for (const auto& srcFilename : DynamicFilePaths)
{
const Poco::File srcFilePath(srcFilename);
- if (!srcFilePath.exists())
+ FileUtil::Stat srcStat(srcFilename);
+ if (!srcStat.exists())
continue;
- const Poco::Path dstFilePath(sysTemplate, srcFilename);
- LOG_DBG("Copying [" << srcFilename << "] -> [" <<
dstFilePath.toString() << "].");
- srcFilePath.copyTo(etcSysTemplatePath);
+ const std::string dstFilename = Poco::Path(sysTemplate,
srcFilename).toString();
+ FileUtil::Stat dstStat(dstFilename);
+
+ // Is it outdated?
+ if (dstStat.isUpToDate(srcStat))
+ {
+ LOG_INF("File [" << dstFilename << "] is already up-to-date.");
+ }
+ else
+ {
+ LOG_INF("Copying [" << srcFilename << "] -> [" << dstFilename
<< "].");
+ if (!FileUtil::copyAtomic(srcFilename, dstFilename, true))
+ {
+ if (!Poco::File(dstFilename).exists()) // stat again.
+ LOG_ERR("Failed to copy [" << srcFilename << "] -> ["
<< dstFilename
+ << "], some functionality
may be missing.");
+ }
+ }
}
}
}
diff --git a/loolwsd-systemplate-setup b/loolwsd-systemplate-setup
index 5b7bda6e8..2f6e462e0 100755
--- a/loolwsd-systemplate-setup
+++ b/loolwsd-systemplate-setup
@@ -21,16 +21,12 @@ cd / || exit 1
# into the template tree of system files for the chroot jails.
# First essential files and shared objects
-find etc/hosts etc/nsswitch.conf etc/resolv.conf \
- etc/passwd etc/group etc/host.conf \
- etc/ld.so.* \
+find etc/ld.so.* \
lib/ld-* lib64/ld-* \
lib/libnss_* lib64/libnss_* lib/*/libnss_* \
lib/libresolv* lib64/libresolv* lib/*/libresolv* \
var/cache/fontconfig \
etc/fonts \
- etc/timezone \
- etc/localtime \
usr/lib/locale/en_US.utf8 \
usr/lib/locale/C.UTF-8 \
usr/lib/locale/locale_archive \
@@ -42,9 +38,6 @@ find etc/hosts etc/nsswitch.conf etc/resolv.conf \
-type f 2>/dev/null
find etc/fonts \
- etc/timezone \
- etc/localtime \
- etc/resolv.conf \
lib/ld-* lib64/ld-* \
lib/libnss_* lib64/libnss_* lib/*/libnss_* \
lib/libresolv* lib64/libresolv* lib/*/libresolv* \
@@ -65,6 +58,9 @@ grep -v dynamic | cut -d " " -f 3 | grep -E '^(/lib|/usr)' |
sort -u | sed -e 's
# This will now copy the file a symlink points to, but whatever.
cpio -p -d -L $CHROOT
+# Remove the dynamic files, which are linked by loolwsd.
+rm -f
$CHROOT/etc/{hosts,nsswitch.conf,resolv.conf,passwd,group,host.conf,timezone,localtime}
+
mkdir -p $CHROOT/lo
mkdir -p $CHROOT/dev
mkdir -p $CHROOT/tmp/dev
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
