Convert function signatures to return statuses.

This is more pre-work to allow ashuffle to re-connect to the MPD server when it goes down. Not all functions yet return the appropriate status, and many still crash on failure. This change also adds a clangd configuration for language server.
joshkunz · May 5, 2023 · e5ef4bd · e5ef4bd
1 parent c8677a6
commit e5ef4bd
Show file tree

Hide file tree

Showing 15 changed files with 369 additions and 212 deletions.
diff --git a/.clangd b/.clangd
@@ -0,0 +1,2 @@
+CompileFlags:
+  CompilationDatabase: build
diff --git a/meson.build b/meson.build
@@ -65,6 +65,26 @@ absl_libs = [
   'absl_time',
   'absl_base',
   'absl_spinlock_wait',
+
+  # Via Status & Time:
+  'absl_cord',
+  'absl_cord_internal',
+  'absl_cordz_functions',
+  'absl_cordz_handle',
+  'absl_cordz_info',
+  'absl_cordz_update_tracker',
+  'absl_debugging_internal',
+  'absl_exponential_biased',
+  'absl_graphcycles_internal',
+  'absl_malloc_internal',
+  'absl_stacktrace',
+  'absl_status',
+  'absl_statusor',
+  'absl_symbolize',
+  'absl_synchronization',
+  'absl_throw_delegate',
+  'absl_time_zone',
+  'absl_demangle_internal',
 ]
 
 absl_deps = []
@@ -131,10 +151,10 @@ libversion = static_library(
 )
 
 sources = files(
-  'src/ashuffle.cc',
-  'src/load.cc',
   'src/args.cc',
+  'src/ashuffle.cc',
   'src/getpass.cc',
+  'src/load.cc',
   'src/log.cc',
   'src/rule.cc',
   'src/shuffle.cc',
@@ -193,13 +213,13 @@ if get_option('tests').enabled()
   ]
 
   tests = {
-    'rule': ['t/rule_test.cc'],
-    'shuffle': ['t/shuffle_test.cc'],
-    'load': ['t/load_test.cc'],
     'args': ['t/args_test.cc'],
     'ashuffle': ['t/ashuffle_test.cc'],
+    'load': ['t/load_test.cc'],
     'log': ['t/log_test.cc'],
     'mpd_fake': ['t/mpd_fake_test.cc'],
+    'rule': ['t/rule_test.cc'],
+    'shuffle': ['t/shuffle_test.cc'],
   }
 
   foreach test_name, test_sources : tests

diff --git a/src/ashuffle.cc b/src/ashuffle.cc
@@ -19,6 +19,7 @@
 #include "args.h"
 #include "ashuffle.h"
 #include "load.h"
+#include "log.h"
 #include "mpd.h"
 #include "mpd_client.h"
 #include "rule.h"
@@ -34,21 +35,41 @@ constexpr std::array<std::string_view, 5> kRequiredCommands = {
     "add", "status", "play", "pause", "idle",
 };
 
-void TryFirst(mpd::MPD *mpd, ShuffleChain *songs) {
-    std::unique_ptr<mpd::Status> status = mpd->CurrentStatus();
+absl::Status TryFirst(mpd::MPD *mpd, ShuffleChain *songs) {
+    absl::StatusOr<std::unique_ptr<mpd::Status>> status = mpd->CurrentStatus();
+    if (!status.ok()) {
+        Log().Error("Failed to query current MPD Status: %s",
+                    status.status().ToString());
+        return status.status();
+    }
+
     // No need to do anything if the player is already going.
-    if (status->IsPlaying()) {
-        return;
+    if ((*status)->IsPlaying()) {
+        return absl::OkStatus();
     }
 
     // If we're not playing, then add a song, and start playing it.
-    mpd->Add(songs->Pick());
+    if (auto s = mpd->Add(songs->Pick()); !s.ok()) {
+        Log().Error("Failed to add song to MPD: %s", s.ToString());
+        return s;
+    }
     // Passing the former queue length, because PlayAt is zero-indexed.
-    mpd->PlayAt(status->QueueLength());
+    if (auto s = mpd->PlayAt((*status)->QueueLength()); !s.ok()) {
+        Log().Error("Failed to play newly added song: %s", s.ToString());
+        return s;
+    }
+    return absl::OkStatus();
 }
 
-void TryEnqueue(mpd::MPD *mpd, ShuffleChain *songs, const Options &options) {
-    std::unique_ptr<mpd::Status> status = mpd->CurrentStatus();
+absl::Status TryEnqueue(mpd::MPD *mpd, ShuffleChain *songs,
+                        const Options &options) {
+    absl::StatusOr<std::unique_ptr<mpd::Status>> status_or =
+        mpd->CurrentStatus();
+    if (!status_or.ok()) {
+        Log().Error("Failed to fetch MPD status");
+        return status_or.status();
+    }
+    std::unique_ptr<mpd::Status> status = std::move(*status_or);
 
     // We're "past" the last song, if there is no current song position.
     bool past_last = !status->SongPosition().has_value();
@@ -89,10 +110,17 @@ void TryEnqueue(mpd::MPD *mpd, ShuffleChain *songs, const Options &options) {
             while (needed > 0) {
                 std::vector<std::string> picked = songs->Pick();
                 needed -= static_cast<int>(picked.size());
-                mpd->Add(picked);
+                if (auto status = mpd->Add(picked); !status.ok()) {
+                    Log().Error("Failed to add picked song: %s",
+                                status.ToString());
+                    return status;
+                }
             }
         } else {
-            mpd->Add(songs->Pick());
+            if (auto status = mpd->Add(songs->Pick()); !status.ok()) {
+                Log().Error("Failed to add picked song: %s", status.ToString());
+                return status;
+            }
         }
     }
 
@@ -102,20 +130,35 @@ void TryEnqueue(mpd::MPD *mpd, ShuffleChain *songs, const Options &options) {
         /* Since the 'status' was before we added our song, and the queue
          * is zero-indexed, the length will be the position of the song we
          * just added. Play that song */
-        mpd->PlayAt(status->QueueLength());
+        if (auto s = mpd->PlayAt(status->QueueLength()); !s.ok()) {
+            Log().Error("Failed to start newly enqueued song: %s",
+                        s.ToString());
+            return s;
+        }
         /* Immediately pause playback if mpd single mode is on */
         if (status->Single()) {
-            mpd->Pause();
+            if (auto status = mpd->Pause(); !status.ok()) {
+                Log().Error("Failed to stop playback in single mode: %s",
+                            status.ToString());
+                return status;
+            }
         }
     }
+    return absl::OkStatus();
 }
 
 void PromptPassword(mpd::MPD *mpd, std::function<std::string()> &getpass_f) {
     /* keep looping till we get a bad error, or we get a good password. */
     while (true) {
         using status = mpd::MPD::PasswordStatus;
         std::string pass = getpass_f();
-        if (mpd->ApplyPassword(pass) == status::kAccepted) {
+        absl::StatusOr<status> result = mpd->ApplyPassword(pass);
+        if (!result.ok()) {
+            std::cerr << "Failed to apply password:" << result.status()
+                      << std::endl;
+            continue;
+        }
+        if (*result == status::kAccepted) {
             return;
         }
         fputs("incorrect password.\n", stderr);
@@ -150,17 +193,21 @@ std::optional<std::unique_ptr<Loader>> Reloader(mpd::MPD *mpd,
 }  // namespace
 
 /* Keep adding songs when the queue runs out */
-void Loop(mpd::MPD *mpd, ShuffleChain *songs, const Options &options,
-          TestDelegate test_d) {
+absl::Status Loop(mpd::MPD *mpd, ShuffleChain *songs, const Options &options,
+                  TestDelegate test_d) {
     static_assert(MPD_IDLE_QUEUE == MPD_IDLE_PLAYLIST,
                   "QUEUE Now different signal.");
     mpd::IdleEventSet set(MPD_IDLE_DATABASE, MPD_IDLE_QUEUE, MPD_IDLE_PLAYER);
 
     // If the test delegate's `skip_init` is set to true, then skip the
     // initializer.
     if (options.tweak.play_on_startup) {
-        TryFirst(mpd, songs);
-        TryEnqueue(mpd, songs, options);
+        if (auto status = TryFirst(mpd, songs); !status.ok()) {
+            return status;
+        }
+        if (auto status = TryEnqueue(mpd, songs, options); !status.ok()) {
+            return status;
+        }
     }
 
     // Tracks if we should be enqueuing new songs.
@@ -169,42 +216,64 @@ void Loop(mpd::MPD *mpd, ShuffleChain *songs, const Options &options,
     // Loop forever if test delegates are not set.
     while (test_d.until_f == nullptr || test_d.until_f()) {
         /* wait till the player state changes */
-        mpd::IdleEventSet events = mpd->Idle(set);
+        absl::StatusOr<mpd::IdleEventSet> events = mpd->Idle(set);
+        if (!events.ok()) {
+            Log().Error("Failed to idle for MPD events: %s",
+                        events.status().ToString());
+            return events.status();
+        }
 
-        if (events.Has(MPD_IDLE_DATABASE) && options.tweak.exit_on_db_update) {
+        if (events->Has(MPD_IDLE_DATABASE) && options.tweak.exit_on_db_update) {
             std::cout << "Database updated, exiting." << std::endl;
             std::exit(0);
         }
 
         /* Only update the database if our original list was built from
          * MPD. */
-        if (events.Has(MPD_IDLE_DATABASE) && options.file_in == nullptr) {
+        if (events->Has(MPD_IDLE_DATABASE) && options.file_in == nullptr) {
             std::optional<std::unique_ptr<Loader>> reloader =
                 Reloader(mpd, options);
             if (reloader.has_value()) {
                 songs->Clear();
                 (*reloader)->Load(songs);
                 PrintChainLength(std::cout, *songs);
             }
-        } else if (events.Has(MPD_IDLE_QUEUE) || events.Has(MPD_IDLE_PLAYER)) {
+        } else if (events->Has(MPD_IDLE_QUEUE) ||
+                   events->Has(MPD_IDLE_PLAYER)) {
             if (options.tweak.suspend_timeout != absl::ZeroDuration()) {
-                std::unique_ptr<mpd::Status> status = mpd->CurrentStatus();
+                auto status_or = mpd->CurrentStatus();
+                if (!status_or.ok()) {
+                    Log().Error("Failed to fetch status in suspend handler");
+                    return status_or.status();
+                }
+                std::unique_ptr<mpd::Status> status = std::move(*status_or);
                 if (status->QueueLength() == 0) {
                     test_d.sleep_f(options.tweak.suspend_timeout);
-                    status = mpd->CurrentStatus();
+                    auto status_or = mpd->CurrentStatus();
+                    if (!status_or.ok()) {
+                        Log().Error(
+                            "Failed to fetch status in suspend handler");
+                        return status_or.status();
+                    }
+                    status = std::move(*status_or);
                     active = status->QueueLength() == 0;
                 }
             }
             if (!active) {
                 continue;
             }
-            TryEnqueue(mpd, songs, options);
+            if (auto status = TryEnqueue(mpd, songs, options); !status.ok()) {
+                Log().Error("Failed regular enqueue");
+                return status;
+            }
         }
     }
+    return absl::OkStatus();
 }
 
-std::unique_ptr<mpd::MPD> Connect(const mpd::Dialer &d, const Options &options,
-                                  std::function<std::string()> &getpass_f) {
+absl::StatusOr<std::unique_ptr<mpd::MPD>> Connect(
+    const mpd::Dialer &d, const Options &options,
+    std::function<std::string()> &getpass_f) {
     /* Attempt to get host from command line if available. Otherwise use
      * MPD_HOST variable if available. Otherwise use 'localhost'. */
     const char *env_host =
@@ -225,13 +294,12 @@ std::unique_ptr<mpd::MPD> Connect(const mpd::Dialer &d, const Options &options,
         .port = mpd_port,
     };
 
-    mpd::Dialer::result r = d.Dial(addr);
-
-    if (std::string *err = std::get_if<std::string>(&r); err != nullptr) {
-        Die("Failed to connect to mpd: %s", *err);
+    absl::StatusOr<std::unique_ptr<mpd::MPD>> r = d.Dial(addr);
+    if (!r.ok()) {
+        Log().Error("Failed to connect to mpd: %s", r.status().ToString());
+        return r.status();
     }
-    std::unique_ptr<mpd::MPD> mpd =
-        std::move(std::get<std::unique_ptr<mpd::MPD>>(r));
+    std::unique_ptr<mpd::MPD> mpd = std::move(*r);
 
     /* Password Workflow:
      * 1. If the user supplied a password, then apply it. No matter what.
@@ -250,19 +318,29 @@ std::unique_ptr<mpd::MPD> Connect(const mpd::Dialer &d, const Options &options,
     // Need a vector for the types to match up.
     const std::vector<std::string_view> required(kRequiredCommands.begin(),
                                                  kRequiredCommands.end());
-    mpd::MPD::Authorization auth = mpd->CheckCommands(required);
-    if (!mpd_host.password && !auth.authorized) {
+    absl::StatusOr<mpd::MPD::Authorization> auth = mpd->CheckCommands(required);
+    if (!auth.ok()) {
+        Log().Error("Failed to check required commands: %s",
+                    auth.status().ToString());
+        return auth.status();
+    }
+    if (!mpd_host.password && !auth->authorized) {
         // If the user did *not* supply a password, and we are missing a
         // required command, then try to prompt the user to provide a password.
         // Once we get/apply a password, try the required commands again...
         PromptPassword(mpd.get(), getpass_f);
         auth = mpd->CheckCommands(required);
+        if (!auth.ok()) {
+            Log().Error("Failed to check required commands: %s",
+                        auth.status().ToString());
+            return auth.status();
+        }
     }
     // If we still can't connect, inform the user which commands are missing,
     // and exit.
-    if (!auth.authorized) {
+    if (!auth->authorized) {
         std::cerr << "Missing MPD Commands:" << std::endl;
-        for (std::string &cmd : auth.missing) {
+        for (std::string &cmd : auth->missing) {
             std::cerr << "  " << cmd << std::endl;
         }
         Die("password applied, but required command still not allowed.");

diff --git a/src/ashuffle.h b/src/ashuffle.h
@@ -6,6 +6,7 @@
 #include <string>
 #include <vector>
 
+#include <absl/status/statusor.h>
 #include <absl/time/clock.h>
 #include <absl/time/time.h>
 #include <mpd/client.h>
@@ -21,8 +22,9 @@ namespace ashuffle {
 // be found in MPD_HOST, then `getpass_f' will be used to prompt the user
 // for a password. If `getpass_f' is NULL, the a default password prompt
 // (based on getpass) will be used.
-std::unique_ptr<mpd::MPD> Connect(const mpd::Dialer& d, const Options& options,
-                                  std::function<std::string()>& getpass_f);
+absl::StatusOr<std::unique_ptr<mpd::MPD>> Connect(
+    const mpd::Dialer& d, const Options& options,
+    std::function<std::string()>& getpass_f);
 
 struct TestDelegate {
     bool (*until_f)() = nullptr;
@@ -33,8 +35,8 @@ struct TestDelegate {
 // queue finishes playing. This is the core loop of `ashuffle`. The tests
 // delegate is used during tests to observe loop effects. It should be set to
 // NULL during normal operations.
-void Loop(mpd::MPD* mpd, ShuffleChain* songs, const Options& options,
-          TestDelegate d = TestDelegate());
+absl::Status Loop(mpd::MPD* mpd, ShuffleChain* songs, const Options& options,
+                  TestDelegate d = TestDelegate());
 
 // Print the size of the database to the given stream, accounting for grouping.
 void PrintChainLength(std::ostream& stream, const ShuffleChain& chain);

diff --git a/src/load.cc b/src/load.cc
@@ -34,7 +34,12 @@ void MPDLoader::Load(ShuffleChain *songs) {
         metadata = mpd::MPD::MetadataOption::kOmit;
     }
 
-    std::unique_ptr<mpd::SongReader> reader = mpd_->ListAll(metadata);
+    auto reader_or = mpd_->ListAll(metadata);
+    if (!reader_or.ok()) {
+        Die("Failed to get reader: %s", reader_or.status().ToString());
+    }
+    std::unique_ptr<mpd::SongReader> reader = std::move(*reader_or);
+
     while (!reader->Done()) {
         std::unique_ptr<mpd::Song> song = *reader->Next();
         if (!Verify(*song)) {

diff --git a/src/log.cc b/src/log.cc
@@ -10,16 +10,13 @@ std::ostream& operator<<(std::ostream& out, const SourceLocation& loc) {
     return out;
 }
 
-void SetOutput(std::ostream& output) {
-    DefaultLogger().SetOutput(output);
-}
+void SetOutput(std::ostream& output) { DefaultLogger().SetOutput(output); }
 
 Logger& DefaultLogger() {
     // static-lifetime logger.
     static Logger logger;
     return logger;
 }
 
-} // namespace log
-} // namespace ashuffle
-
+}  // namespace log
+}  // namespace ashuffle