[Libreoffice-commits] online.git: loolwsd/debian loolwsd/LOOLBroker.cpp loolwsd/LOOLKit.cpp loolwsd/loolwsd.spec.in loolwsd/Makefile.am loolwsd/README.vars

Tor Lillqvist tml at collabora.com
Mon Apr 4 06:09:52 UTC 2016


 loolwsd/LOOLBroker.cpp          |  115 ++++++++++++----------------------------
 loolwsd/LOOLKit.cpp             |   83 ----------------------------
 loolwsd/Makefile.am             |   18 ++----
 loolwsd/README.vars             |   19 ++----
 loolwsd/debian/loolwsd.postinst |    1 
 loolwsd/loolwsd.spec.in         |    2 
 6 files changed, 50 insertions(+), 188 deletions(-)

New commits:
commit a132e06409353b374e754a9aecf6b55b06778118
Author: Tor Lillqvist <tml at collabora.com>
Date:   Mon Apr 4 08:26:05 2016 +0300

    Bin the non-preinit and non-fork code paths
    
    Preiniting LibreOfficeKit and forking kit processes (instead of
    spawning) has worked fine for a while, and has been the default way
    this works.
    
    No 'loolkit' program gets built any more.

diff --git a/loolwsd/LOOLBroker.cpp b/loolwsd/LOOLBroker.cpp
index c37116a..defcfae 100644
--- a/loolwsd/LOOLBroker.cpp
+++ b/loolwsd/LOOLBroker.cpp
@@ -21,7 +21,6 @@
 // we can avoid execve and share lots of memory here. We
 // can't link to a non-PIC translation unit though, so
 // include to share.
-#define LOOLKIT_NO_MAIN 1
 #include "LOOLKit.cpp"
 
 #define LIB_SOFFICEAPP  "lib" "sofficeapp" ".so"
@@ -34,7 +33,6 @@ const std::string BROKER_PREFIX = "lokit";
 
 static int ReaderBroker = -1;
 
-static std::string LoolkitPath;
 static std::atomic<unsigned> ForkCounter;
 static unsigned int ChildCounter = 0;
 static int NumPreSpawnedChildren = 1;
@@ -74,7 +72,7 @@ private:
 };
 
 /// Initializes LibreOfficeKit for cross-fork re-use.
-static bool globalPreinit(const std::string &loTemplate)
+static void globalPreinit(const std::string &loTemplate)
 {
     const std::string libSofficeapp = loTemplate + "/program/" LIB_SOFFICEAPP;
 
@@ -85,8 +83,8 @@ static bool globalPreinit(const std::string &loTemplate)
         handle = dlopen(libSofficeapp.c_str(), RTLD_GLOBAL|RTLD_NOW);
         if (!handle)
         {
-            Log::warn("Failed to load " + libSofficeapp + ": " + std::string(dlerror()));
-            return false;
+            Log::error("Failed to load " + libSofficeapp + ": " + std::string(dlerror()));
+            _exit(Application::EXIT_SOFTWARE);
         }
         loadedLibrary = libSofficeapp;
     }
@@ -98,30 +96,33 @@ static bool globalPreinit(const std::string &loTemplate)
             handle = dlopen(libMerged.c_str(), RTLD_GLOBAL|RTLD_NOW);
             if (!handle)
             {
-                Log::warn("Failed to load " + libMerged + ": " + std::string(dlerror()));
-                return false;
+                Log::error("Failed to load " + libMerged + ": " + std::string(dlerror()));
+                _exit(Application::EXIT_SOFTWARE);
             }
             loadedLibrary = libMerged;
         }
         else
         {
-            Log::warn("Neither " + libSofficeapp + " or " + libMerged + " exist.");
-            return false;
+            Log::error("Neither " + libSofficeapp + " or " + libMerged + " exist.");
+            _exit(Application::EXIT_SOFTWARE);
         }
     }
 
     LokHookPreInit* preInit = (LokHookPreInit *)dlsym(handle, "lok_preinit");
     if (!preInit)
     {
-        Log::warn("Note: No lok_preinit hook in " + loadedLibrary);
-        return false;
+        Log::error("No lok_preinit symbol in " + loadedLibrary);
+        _exit(Application::EXIT_SOFTWARE);
     }
 
-    return preInit((loTemplate + "/program").c_str(), "file:///user") == 0;
+    if (preInit((loTemplate + "/program").c_str(), "file:///user") != 0)
+    {
+        Log::error("lok_preinit() in " + loadedLibrary + " failed");
+        _exit(Application::EXIT_SOFTWARE);
+    }
 }
 
-static int createLibreOfficeKit(const bool sharePages,
-                                const std::string& childRoot,
+static int createLibreOfficeKit(const std::string& childRoot,
                                 const std::string& sysTemplate,
                                 const std::string& loTemplate,
                                 const std::string& loSubPath)
@@ -137,56 +138,28 @@ static int createLibreOfficeKit(const bool sharePages,
         return -1;
     }
 
-    if (sharePages)
-    {
-        Log::debug("Forking LibreOfficeKit.");
+    Log::debug("Forking LibreOfficeKit.");
 
-        Process::PID pid;
-        if (!(pid = fork()))
-        {
-            // child
-            if (std::getenv("SLEEPKITFORDEBUGGER"))
-            {
-                std::cerr << "Sleeping " << std::getenv("SLEEPKITFORDEBUGGER")
-                          << " seconds to give you time to attach debugger to process "
-                          << Process::id() << std::endl;
-                Thread::sleep(std::stoul(std::getenv("SLEEPKITFORDEBUGGER")) * 1000);
-            }
-
-            lokit_main(childRoot, sysTemplate, loTemplate, loSubPath, pipeKit);
-            _exit(Application::EXIT_OK);
-        }
-        else
+    Process::PID pid;
+    if (!(pid = fork()))
+    {
+        // child
+        if (std::getenv("SLEEPKITFORDEBUGGER"))
         {
-            // parent
-            childPID = pid; // (somehow - switch the hash to use real pids or ?) ...
-            Log::info("Forked kit [" + std::to_string(childPID) + "].");
+            std::cerr << "Sleeping " << std::getenv("SLEEPKITFORDEBUGGER")
+                      << " seconds to give you time to attach debugger to process "
+                      << Process::id() << std::endl;
+            Thread::sleep(std::stoul(std::getenv("SLEEPKITFORDEBUGGER")) * 1000);
         }
+
+        lokit_main(childRoot, sysTemplate, loTemplate, loSubPath, pipeKit);
+        _exit(Application::EXIT_OK);
     }
     else
     {
-        Process::Args args;
-        args.push_back("--childroot=" + childRoot);
-        args.push_back("--systemplate=" + sysTemplate);
-        args.push_back("--lotemplate=" + loTemplate);
-        args.push_back("--losubpath=" + loSubPath);
-        args.push_back("--pipe=" + pipeKit);
-        args.push_back("--clientport=" + std::to_string(ClientPortNumber));
-
-        Log::info("Launching LibreOfficeKit #" + std::to_string(ChildCounter) +
-                  ": " + LoolkitPath + " " +
-                  Poco::cat(std::string(" "), args.begin(), args.end()));
-
-        Poco::ProcessHandle procChild = Process::launch(LoolkitPath, args);
-        childPID = procChild.id();
-        Log::info("Spawned kit [" + std::to_string(childPID) + "].");
-
-        if (!Process::isRunning(procChild))
-        {
-            // This can happen if we fail to copy it, or bad chroot etc.
-            Log::error("Error: loolkit [" + std::to_string(childPID) + "] was stillborn.");
-            return -1;
-        }
+        // parent
+        childPID = pid; // (somehow - switch the hash to use real pids or ?) ...
+        Log::info("Forked kit [" + std::to_string(childPID) + "].");
     }
 
     Log::info() << "Created Kit #" << ChildCounter << ", PID: " << childPID << Log::end;
@@ -289,8 +262,6 @@ int main(int argc, char** argv)
         }
     }
 
-    LoolkitPath = Poco::Path(argv[0]).parent().toString() + "loolkit";
-
     if (loSubPath.empty() || sysTemplate.empty() ||
         loTemplate.empty() || childRoot.empty() ||
         NumPreSpawnedChildren < 1)
@@ -307,19 +278,13 @@ int main(int argc, char** argv)
 
     setupPipes(childRoot);
 
-    // Initialize LoKit and hope we can fork and save memory by sharing pages.
-    const bool sharePages = std::getenv("LOK_NO_PREINIT") == nullptr
-                          ? globalPreinit(loTemplate)
-                          : std::getenv("LOK_FORK") != nullptr;
+    // Initialize LoKit
+    globalPreinit(loTemplate);
 
-    if (!sharePages)
-        Log::warn("Cannot fork, will spawn instead.");
-    else
-        Log::info("Preinit stage OK.");
+    Log::info("Preinit stage OK.");
 
     // We must have at least one child, more is created dynamically.
-    if (createLibreOfficeKit(sharePages, childRoot, sysTemplate,
-                             loTemplate, loSubPath) < 0)
+    if (createLibreOfficeKit(childRoot, sysTemplate, loTemplate, loSubPath) < 0)
     {
         Log::error("Error: failed to create children.");
         std::exit(Application::EXIT_SOFTWARE);
@@ -328,13 +293,6 @@ int main(int argc, char** argv)
     if (NumPreSpawnedChildren > 1)
         ForkCounter = NumPreSpawnedChildren - 1;
 
-    if (!sharePages)
-    {
-        dropCapability(CAP_SYS_CHROOT);
-        dropCapability(CAP_MKNOD);
-        dropCapability(CAP_FOWNER);
-    }
-
     ChildDispatcher childDispatcher;
     Log::info("loolbroker is ready.");
 
@@ -357,8 +315,7 @@ int main(int argc, char** argv)
             size_t newInstances = 0;
             do
             {
-                if (createLibreOfficeKit(sharePages, childRoot, sysTemplate,
-                                         loTemplate, loSubPath) < 0)
+                if (createLibreOfficeKit(childRoot, sysTemplate, loTemplate, loSubPath) < 0)
                 {
                     Log::error("Error: fork failed.");
                 }
diff --git a/loolwsd/LOOLKit.cpp b/loolwsd/LOOLKit.cpp
index 23b6063..c45aebc 100644
--- a/loolwsd/LOOLKit.cpp
+++ b/loolwsd/LOOLKit.cpp
@@ -8,7 +8,7 @@
  */
 
 /*
- * NB. this file is compiled both standalone, and as part of the LOOLBroker.
+ * NB. this file is compiled as part of the LOOLBroker.
  */
 
 #include <sys/prctl.h>
@@ -855,11 +855,9 @@ void lokit_main(const std::string& childRoot,
                 const std::string& pipe,
                 bool doBenchmark = false)
 {
-#ifdef LOOLKIT_NO_MAIN
     // Reinitialize logging when forked.
     Log::initialize("kit");
     Util::rng::reseed();
-#endif
 
     assert(!childRoot.empty());
     assert(!sysTemplate.empty());
@@ -907,7 +905,6 @@ void lokit_main(const std::string& childRoot,
 
         File(jailPath).createDirectories();
 
-#ifdef LOOLKIT_NO_MAIN
         // Create a symlink inside the jailPath so that the absolute pathname loTemplate, when
         // interpreted inside a chroot at jailPath, points to loSubPath (relative to the chroot).
         Path symlinkSource(jailPath, Path(loTemplate.substr(1)));
@@ -925,7 +922,6 @@ void lokit_main(const std::string& childRoot,
             Log::error("Error: symlink(\"" + symlinkTarget + "\",\"" + symlinkSource.toString() + "\") failed");
             throw Poco::Exception("symlink() failed");
         }
-#endif
 
         Path jailLOInstallation(jailPath, loSubPath);
         jailLOInstallation.makeDirectory();
@@ -1108,81 +1104,4 @@ void lokit_main(const std::string& childRoot,
     Log::info("Process [" + process_name + "] finished.");
 }
 
-#ifndef LOOLKIT_NO_MAIN
-
-/// Simple argument parsing wrapper / helper for the above.
-int main(int argc, char** argv)
-{
-    if (std::getenv("SLEEPFORDEBUGGER"))
-    {
-        std::cerr << "Sleeping " << std::getenv("SLEEPFORDEBUGGER")
-                  << " seconds to attach debugger to process "
-                  << Process::id() << std::endl;
-        Thread::sleep(std::stoul(std::getenv("SLEEPFORDEBUGGER")) * 1000);
-    }
-
-    Log::initialize("kit");
-
-    std::string childRoot;
-    std::string sysTemplate;
-    std::string loTemplate;
-    std::string loSubPath;
-    std::string pipe;
-
-    for (int i = 1; i < argc; ++i)
-    {
-        char *cmd = argv[i];
-        char *eq;
-
-        if (std::strstr(cmd, "--childroot=") == cmd)
-        {
-            eq = std::strchr(cmd, '=');
-            childRoot = std::string(eq+1);
-        }
-        else if (std::strstr(cmd, "--systemplate=") == cmd)
-        {
-            eq = std::strchr(cmd, '=');
-            sysTemplate = std::string(eq+1);
-        }
-        else if (std::strstr(cmd, "--lotemplate=") == cmd)
-        {
-            eq = std::strchr(cmd, '=');
-            loTemplate = std::string(eq+1);
-        }
-        else if (std::strstr(cmd, "--losubpath=") == cmd)
-        {
-            eq = std::strchr(cmd, '=');
-            loSubPath = std::string(eq+1);
-        }
-        else if (std::strstr(cmd, "--pipe=") == cmd)
-        {
-            eq = std::strchr(cmd, '=');
-            pipe = std::string(eq+1);
-        }
-        else if (std::strstr(cmd, "--clientport=") == cmd)
-        {
-            eq = std::strchr(cmd, '=');
-            ClientPortNumber = std::stoll(std::string(eq+1));
-        }
-    }
-
-    if (loSubPath.empty())
-    {
-        Log::error("Error: --losubpath is empty");
-        std::exit(Application::EXIT_SOFTWARE);
-    }
-
-    if (pipe.empty())
-    {
-        Log::error("Error: --pipe is empty");
-        std::exit(Application::EXIT_SOFTWARE);
-    }
-
-    lokit_main(childRoot, sysTemplate, loTemplate, loSubPath, pipe);
-
-    return Application::EXIT_OK;
-}
-
-#endif
-
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/loolwsd/Makefile.am b/loolwsd/Makefile.am
index 7056792..4a0d753 100644
--- a/loolwsd/Makefile.am
+++ b/loolwsd/Makefile.am
@@ -1,6 +1,6 @@
 SUBDIRS = test
 
-bin_PROGRAMS = loolwsd loolbroker loolkit loolmap loolmount
+bin_PROGRAMS = loolwsd loolbroker loolmap loolmount
 
 dist_bin_SCRIPTS = loolwsd-systemplate-setup discovery.xml
 
@@ -47,9 +47,6 @@ lokitclient_SOURCES = IoUtil.cpp \
 broker_shared_sources = ChildProcessSession.cpp \
                         $(shared_sources)
 
-loolkit_SOURCES = LOOLKit.cpp \
-                  $(broker_shared_sources)
-
 loolbroker_SOURCES = LOOLBroker.cpp \
                      $(broker_shared_sources)
 
@@ -92,16 +89,15 @@ clean-cache:
 # Intentionally don't use "*" below... Avoid risk of accidentally running rm -rf /*
 	test -n "@LOOLWSD_CACHEDIR@" && rm -rf "@LOOLWSD_CACHEDIR@"/[0-9a-f]
 
-# After building loolbroker and loolkit, set their capabilities as
-# required. Do it already after a plain 'make' to allow for testing
-# without installing. When building for packaging, no need for this,
-# as the capabilities won't survive packaging anyway. Instead, handle
-# it when installing the RPM or Debian package.
+# After building loolbroker, set its capabilities as required. Do it
+# already after a plain 'make' to allow for testing without
+# installing. When building for packaging, no need for this, as the
+# capabilities won't survive packaging anyway. Instead, handle it when
+# installing the RPM or Debian package.
 
-all-local: loolbroker loolkit certificates
+all-local: loolbroker certificates
 	if test "$$BUILDING_FROM_RPMBUILD" != yes; then \
 	    sudo @SETCAP@ cap_fowner,cap_mknod,cap_sys_chroot=ep loolbroker; \
-	    sudo @SETCAP@ cap_fowner,cap_mknod,cap_sys_chroot=ep loolkit; \
 	    sudo @SETCAP@ cap_sys_admin=ep loolmount; \
 	    echo "Set required capabilities"; \
 	else \
diff --git a/loolwsd/README.vars b/loolwsd/README.vars
index 59108ab..e87f3ab 100644
--- a/loolwsd/README.vars
+++ b/loolwsd/README.vars
@@ -16,18 +16,11 @@ LOOL_LOGLEVEL           <level>
 		error, warning, notice, information, debug,
 		trace
 
-LOK_NO_PREINIT          <set/unset>
-        set this to disable pre-initialization of LOK instances.
-
-LOK_FORK                <set/unset>
-	set this to enable forking instead of execve'ing of kit
-	process instances even if LOK_NO_PREINIT is set.
-
 SLEEPFORDEBUGGER        <seconds to sleep>
-        sleep <n> seconds while launching processes in order to
-        allow a 'sudo gdb' session to 'attach <pid>' to them.
+        sleep <n> seconds in the broken process after starting in
+        order to allow a 'sudo gdb' session to 'attach <pid>' to them.
 
-SLEEPKITFORDEBUGGER      <seconds to sleep>
-        sleep <n> seconds after launching (or forking) each
-        kit process instance, to allow a 'sudo gdb' session
-        to attach and debug that process.
+SLEEPKITFORDEBUGGER     <seconds to sleep>
+        sleep <n> seconds in each kit process instance after forking,
+        to allow a 'sudo gdb' session to attach and debug that
+        process.
diff --git a/loolwsd/debian/loolwsd.postinst b/loolwsd/debian/loolwsd.postinst
index bb4f6da..d2b50ec 100755
--- a/loolwsd/debian/loolwsd.postinst
+++ b/loolwsd/debian/loolwsd.postinst
@@ -4,7 +4,6 @@ set -e
 
 case "$1" in
     configure)
-	setcap cap_fowner,cap_mknod,cap_sys_chroot=ep /usr/bin/loolkit || true
 	setcap cap_fowner,cap_mknod,cap_sys_chroot=ep /usr/bin/loolbroker || true
 
 	adduser --quiet --system --group --home /opt/lool lool
diff --git a/loolwsd/loolwsd.spec.in b/loolwsd/loolwsd.spec.in
index 9d06a2f..9c0e198 100644
--- a/loolwsd/loolwsd.spec.in
+++ b/loolwsd/loolwsd.spec.in
@@ -57,7 +57,6 @@ echo "0 0 */1 * * root find /var/cache/loolwsd -name \"*.png\" -a -atime +10 -ex
 /usr/bin/loolwsd
 /usr/bin/loolwsd-systemplate-setup
 /usr/bin/loolmap
-/usr/bin/loolkit
 /usr/bin/loolbroker
 /usr/bin/discovery.xml
 %{_unitdir}/loolwsd.service
@@ -71,7 +70,6 @@ echo "0 0 */1 * * root find /var/cache/loolwsd -name \"*.png\" -a -atime +10 -ex
 
 %post
 setcap cap_fowner,cap_mknod,cap_sys_chroot=ep /usr/bin/loolbroker
-setcap cap_fowner,cap_mknod,cap_sys_chroot=ep /usr/bin/loolkit
 
 getent group %{group} >/dev/null || groupadd -r %{group}
 getent passwd %{owner} >/dev/null || useradd -g %{group} -r %{owner}


More information about the Libreoffice-commits mailing list