Merge lp:~thomas-voss/location-service/fix-1447110 into lp:location-service/15.04

[Thomas Voß]

Log to stderr by default, relying on upstart to rotate logs appropriately.

Log to stderr by default, relying on upstart to rotate logs appropriately.

[lp:~thomas-voss/location-service/fix-1447110]

222. By Thomas Voß on 2016-03-29

Dial down logging of issues in communicating updates to clients.

[Scott Sweeny (ssweeny)]

LGTM

review: Approve

[lp:~thomas-voss/location-service/fix-1447110]

223. By Thomas Voß on 2016-03-30

Remove obsolete /var/log/ubuntu-location-service on startup.

224. By Thomas Voß on 2016-03-31

Remove directory even if not empty (ensure consistent semantics across boost versions).

225. By Thomas Voß on 2016-03-31

Add test ensuring correct behavior.

=== modified file 'doc/debugging.md'
--- doc/debugging.md	2015-10-20 12:02:02 +0000
+++ doc/debugging.md	2016-03-31 10:16:15 +0000
@@ -54,7 +54,6 @@
 sequence to add some env vars:
 
     export GLOG_v=200
-    export GLOG_logtostderr=1
 
 before the exec. Reboot, and start some app. You should have some log
 files under /var/log/upstart/ubuntu-espoo-service.log and

=== modified file 'src/location_service/com/ubuntu/location/service/daemon.cpp'
--- src/location_service/com/ubuntu/location/service/daemon.cpp	2016-01-06 16:15:26 +0000
+++ src/location_service/com/ubuntu/location/service/daemon.cpp	2016-03-31 10:16:15 +0000
@@ -42,6 +42,7 @@
 #include <core/posix/signal.h>
 
 #include <boost/asio.hpp>
+#include <boost/filesystem.hpp>
 
 #include <system_error>
 #include <thread>
@@ -170,6 +171,20 @@
 
 int location::service::Daemon::main(const location::service::Daemon::Configuration& config)
 {
+    // Ensure that log files dating back to before the fix
+    // for lp:1447110 are removed and do not waste space.
+    {
+        static const boost::filesystem::path old_log_dir{"/var/log/ubuntu-location-service"};
+        boost::system::error_code ec;
+        boost::filesystem::remove_all(old_log_dir, ec);
+    }
+    // Setup logging for the daemon.
+    FLAGS_logtostderr = true;
+    FLAGS_stop_logging_if_full_disk = true;
+    FLAGS_max_log_size = 5;
+
+    google::InitGoogleLogging("com.ubuntu.location");
+
     auto trap = core::posix::trap_signals_for_all_subsequent_threads(
     {
         core::posix::Signal::sig_term,

=== modified file 'src/location_service/com/ubuntu/location/service/daemon_main.cpp'
--- src/location_service/com/ubuntu/location/service/daemon_main.cpp	2015-01-13 16:35:20 +0000
+++ src/location_service/com/ubuntu/location/service/daemon_main.cpp	2016-03-31 10:16:15 +0000
@@ -18,41 +18,10 @@
 
 #include "daemon.h"
 
-#include <com/ubuntu/location/logging.h>
-
-#include <boost/filesystem.hpp>
-
 namespace location = com::ubuntu::location;
 
-namespace
-{
-// The directory we put log files in.
-constexpr const char* log_dir{"/var/log/ubuntu-location-service"};
-}
-
 int main(int argc, const char** argv)
 {
-    // Setup logging for the daemon.
-    boost::system::error_code ec;
-    boost::filesystem::create_directories(boost::filesystem::path{log_dir}, ec);
-
-    // According to http://www.boost.org/doc/libs/1_55_0/libs/filesystem/doc/reference.html#create_directories
-    //   Creation failure because p resolves to an existing directory shall not be treated as an error.
-    // With that, we are free to check the error condition and adjust to stderr
-    // logging accordingly.
-    if (ec)
-    {
-        FLAGS_logtostderr = true;
-        VLOG(1) << "Problem creating directory for log files: " << ec << "."
-                << "Falling back to stderr logging.";
-    }
-
-    FLAGS_log_dir = log_dir;
-    FLAGS_stop_logging_if_full_disk = true;
-    FLAGS_max_log_size = 5;
-
-    google::InitGoogleLogging("com.ubuntu.location");
-
     location::service::Daemon::Configuration config;
     try
     {

=== modified file 'src/location_service/com/ubuntu/location/service/session/skeleton.cpp'
--- src/location_service/com/ubuntu/location/service/session/skeleton.cpp	2015-10-22 19:32:08 +0000
+++ src/location_service/com/ubuntu/location/service/session/skeleton.cpp	2016-03-31 10:16:15 +0000
@@ -287,7 +287,7 @@
         {
             if (result.is_error())
             {
-                LOG(INFO) << "Failed to communicate position update to client: " << result.error().print();    
+                VLOG(10) << "Failed to communicate position update to client: " << result.error().print();
             }
         }, position);
     } catch(const std::exception&)
@@ -310,7 +310,7 @@
         {
             if (result.is_error())
             {
-                LOG(INFO) << "Failed to communicate position update to client: " << result.error().print();    
+                VLOG(10) << "Failed to communicate heading update to client: " << result.error().print();
             }
         }, heading);
     } catch(const std::exception&)
@@ -333,7 +333,7 @@
         {
             if (result.is_error())
             {
-                LOG(INFO) << "Failed to communicate position update to client: " << result.error().print();    
+                VLOG(10) << "Failed to communicate velocity update to client: " << result.error().print();
             }
         }, velocity);
     } catch(const std::exception&)

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2016-02-01 15:58:06 +0000
+++ tests/CMakeLists.txt	2016-03-31 10:16:15 +0000
@@ -127,3 +127,5 @@
 LOCATION_SERVICE_ADD_TEST(remote_provider_test remote_provider_test.cpp)
 LOCATION_SERVICE_ADD_TEST(espoo_provider_test espoo_provider_test.cpp)
 LOCATION_SERVICE_ADD_TEST(delayed_service_test delayed_service_test.cpp)
+
+LOCATION_SERVICE_ADD_TEST(bug_1447110 bug_1447110.cpp)

=== added file 'tests/bug_1447110.cpp'
--- tests/bug_1447110.cpp	1970-01-01 00:00:00 +0000
+++ tests/bug_1447110.cpp	2016-03-31 10:16:15 +0000
@@ -0,0 +1,66 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Thomas Voß <[e-mail]>
+ */
+
+#include <com/ubuntu/location/service/daemon.h>
+
+#include <core/posix/fork.h>
+
+#include <gtest/gtest.h>
+#include <boost/filesystem.hpp>
+
+namespace location = com::ubuntu::location;
+
+namespace
+{
+auto null_dbus_connection_factory = [](core::dbus::WellKnownBus)
+{
+    return core::dbus::Bus::Ptr{};
+};
+}
+
+TEST(Bug1447110, obsolete_log_directory_is_removed_on_startup)
+{
+    // We are trying to establish the pre-condition that a non-empty
+    // directory /var/log/ubuntu-location-service exists. If we fail to do
+    // so, we leave a message and bail out, not marking the test as failure.
+    {
+        boost::system::error_code ec;
+        boost::filesystem::remove_all("/var/log/ubuntu-location-service/not_empty", ec);
+        if (not boost::filesystem::create_directories("/var/log/ubuntu-location-service/not_empty", ec) || ec)
+        {
+            std::cerr << "Unable to create directory for testing purposes." << std::endl;
+            return;
+        }
+    }
+
+    auto cp = core::posix::fork([]()
+    {
+        const char* args[] =
+        {
+            "--bus", "session",
+            "--testing"
+        };
+        auto config = location::service::Daemon::Configuration::from_command_line_args(3, args, null_dbus_connection_factory);
+        location::service::Daemon::main(config);
+
+        return core::posix::exit::Status::success;
+    }, core::posix::StandardStream::stderr);
+
+    std::this_thread::sleep_for(std::chrono::seconds{1});
+    EXPECT_FALSE(boost::filesystem::exists("/var/log/ubuntu-location-service"));
+}