From a98aacaca3c6ec9a06b42de0a7921b3604f5264d Mon Sep 17 00:00:00 2001 From: Michael Herstine Date: Sat, 11 Dec 2021 08:56:43 -0800 Subject: [PATCH] Remove Snafu. After some thought & some research, I decided I want to hand-craft my Errors. Thoughts here: - Snafu has been removed - `cargo test` passes - `cargo doc` builds cleanly --- ChangeLog | 12 + NEWS | 97 ++-- README.org | 24 +- admin/Dockerfile-arch | 26 +- admin/Dockerfile-debian | 4 +- admin/build-dev-arch-pkg-cnt | 5 + admin/build-dev-debian-pkg-cnt | 5 + configure.ac | 2 +- doc/version.texi | 4 +- mpdpopm/Cargo.toml.in | 5 +- mpdpopm/Makefile.am | 1 - mpdpopm/src/bin/mppopm.rs | 244 ++++++---- mpdpopm/src/bin/mppopmd.rs | 167 +++---- mpdpopm/src/clients.rs | 798 +++++++++++++++++---------------- mpdpopm/src/commands.rs | 168 ++++--- mpdpopm/src/config.rs | 32 +- mpdpopm/src/error_from.rs | 29 -- mpdpopm/src/filters_ast.rs | 330 +++++++------- mpdpopm/src/lib.rs | 209 +++++---- mpdpopm/src/messages.rs | 265 ++++++++--- mpdpopm/src/playcounts.rs | 122 +++-- mpdpopm/src/ratings.rs | 116 +++-- 22 files changed, 1576 insertions(+), 1089 deletions(-) delete mode 100644 mpdpopm/src/error_from.rs diff --git a/ChangeLog b/ChangeLog index 6c817b5..6188c71 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +2021-12-16 Michael Herstine + + Remove Snafu. + After some thought & some research, I decided I want to hand-craft + my Errors. Thoughts here: + + + + - Snafu has been removed + - `cargo test` passes + - `cargo doc` builds cleanly + 2021-10-11 Michael Herstine Address issue #5. diff --git a/NEWS b/NEWS index 5aaa9c4..4b7d8c6 100644 --- a/NEWS +++ b/NEWS @@ -1,112 +1,121 @@ mpdpopm News -- history of user-visible changes -*- outline -*- -* 0.3.0 build +* 0.3 builds -** Bugfixes +** 0.3.1 build -*** Address Issue #5: Cannot connect to mpd unix socket -* 0.2.3 build +No user-visible changes; strictly a hygiene build (re-vamped error-handling throughout). +** 0.3.0 build -** User-visible changes +*** Bugfixes -*** broke-up the README +**** Address Issue #5: Cannot connect to mpd unix socket +* 0.2 builds + +** 0.2.3 build + +*** User-visible changes + +**** broke-up the README Broke-up README.org into a much shorter version focused on installing mpdpopm and a full-fledged user-manual. -** Bug-fixes +*** Bug-fixes -*** quoting +**** quoting Fixed a number of bugs relating to quoting filters. -* 0.2.2 build +** 0.2.2 build -** User-visible changes +*** User-visible changes -*** `searchadd' command fully working +**** `searchadd' command fully working -*** `mppopm' now has findadd & searchadd commands -* 0.2.1 build +**** `mppopm' now has findadd & searchadd commands +** 0.2.1 build -** User-visible changes +*** User-visible changes -*** `findadd' command fully working -* 0.2.0 build +**** `findadd' command fully working +** 0.2.0 build -** User-visible changes +*** User-visible changes -*** `findadd' command +**** `findadd' command mpdpopm now supports a `findadd' command; just like the MPD command by the same name, but this implementation includes terms for information managed by mpdpopm (so you can search for tracks with a rating > 3 stars, for instance). -* 0.1.15 build +* 0.1 builds + +** 0.1.15 build -** User-visible changes +*** User-visible changes -*** Include sample configuration & systemd unit files +**** Include sample configuration & systemd unit files -*** Begin providing an Arch package +**** Begin providing an Arch package -* 0.1.14 build +** 0.1.14 build -** User-visible changes +*** User-visible changes -*** Complete re-write of the README +**** Complete re-write of the README -*** Releases will now contain pre-built binaries & a Debian binary package -* 0.1.13 build +**** Releases will now contain pre-built binaries & a Debian binary package +** 0.1.13 build -** Bugfixes +*** Bugfixes -*** Issue 1: `get_messages` fails on repeated channels -* 0.1.12 build +**** Issue 1: `get_messages` fails on repeated channels +** 0.1.12 build Minor changes preparatory to publication on crates.io; no user-facing changes. -* 0.1.11 build +** 0.1.11 build -** User-visible changes +*** User-visible changes - scribbu feature replaced with a generalized command facility -* 0.1.10 build +** 0.1.10 build -** User-visible changes +*** User-visible changes - 'mppopm set-genre' now takes free-form text rather than a numeric genre -* 0.1.9 build +** 0.1.9 build -** User-visible changes +*** User-visible changes - removed the "sent-to-playlist" command -** Bugfixes +*** Bugfixes - fixed: failure in message processing caused the daemon to exit -* 0.1.8 build: daemon support +** 0.1.8 build: daemon support - added support for running `mppopmd' as a daemon -* 0.1.7 build: re-factoring & bug fixes +** 0.1.7 build: re-factoring & bug fixes - added support for commands to signal the need for an update on completion - fixed bug in in the "update" command processing -* 0.1.6 build: new server-side commands +** 0.1.6 build: new server-side commands Gated behind the "scribbu" feature flag - setgenre - setxtag -* 0.1.5 build: new server-side commands +** 0.1.5 build: new server-side commands - setpc - setlp - send -* 0.1.4 build: client features, daemon bug fixes +** 0.1.4 build: client features, daemon bug fixes -* 0.1.3 build: complete re-factor +** 0.1.3 build: complete re-factor - split the code into separate daemon & cli -* 0.1.2 build: No news, yet! +** 0.1.2 build: No news, yet! diff --git a/README.org b/README.org index 65d381f..f460b46 100644 --- a/README.org +++ b/README.org @@ -2,7 +2,7 @@ #+AUTHOR: Michael Herstine #+DESCRIPTION: mpdpopm #+EMAIL: sp1ff@pobox.com -#+DATE: <2021-10-11 Mon 12:10> +#+DATE: <2021-12-16 Thu 18:03> #+AUTODATE: t * Introduction @@ -56,10 +56,10 @@ Thanks to a suggestion by [[https://github.com/m040601][m040601]], you can down #+BEGIN_SRC bash cd /tmp -curl -L --output mpdpopm-0.3.0.tar.gz https://github.com/sp1ff/mpdpopm/releases/download/0.3.0/mpdpopm-0.3.0-x86_64-unknown-linux.tar.gz -tar xf mpdpopm-0.3.0.tar.gz -tree mpdpopm-0.3.0-x86_64-unknown-linux/ -mpdpopm-0.3.0-x86_64-unknown-linux/ +curl -L --output mpdpopm-0.3.1.tar.gz https://github.com/sp1ff/mpdpopm/releases/download/0.3.1/mpdpopm-0.3.1-x86_64-unknown-linux.tar.gz +tar xf mpdpopm-0.3.1.tar.gz +tree mpdpopm-0.3.1-x86_64-unknown-linux/ +mpdpopm-0.3.1-x86_64-unknown-linux/ ├── bin │ ├── mppopm │ └── mppopmd @@ -89,8 +89,8 @@ If you're running on a Debian-based Linux distribution, and you're on an x86_64 #+BEGIN_SRC bash cd /tmp -curl -L -O https://github.com/sp1ff/mpdpopm/releases/download/0.3.0/mpdpopm_0.3.0_amd64.deb -sudo dpkg -i mpdpopm_0.3.0_amd64.deb +curl -L -O https://github.com/sp1ff/mpdpopm/releases/download/0.3.1/mpdpopm_0.3.1_amd64.deb +sudo dpkg -i mpdpopm_0.3.1_amd64.deb #+END_SRC The binaries will be placed in =/usr/local/bin=, and you can proceed to [[#getting_started][Getting Started]], below. @@ -101,8 +101,8 @@ If you're running on an Arch-based Linux distribution, and you're on an x86_64 p #+BEGIN_SRC bash cd /tmp -curl -L -O https://github.com/sp1ff/mpdpopm/releases/download/0.3.0/mpdpopm_0.3.0-1-x86_64.pkg.tar.zst -sudo pacman -U mpdpopm_0.3.0-1-x86_64.pkg.tar.zst +curl -L -O https://github.com/sp1ff/mpdpopm/releases/download/0.3.1/mpdpopm_0.3.1-1-x86_64.pkg.tar.zst +sudo pacman -U mpdpopm_0.3.1-1-x86_64.pkg.tar.zst #+END_SRC The binaries will be placed in =/usr/local/bin=, and you can proceed to [[#getting_started][Getting Started]], below. @@ -113,9 +113,9 @@ If you've got the Rust toolchain as well as Autotools installed, you can build f #+BEGIN_SRC bash cd /tmp -curl -L -O https://github.com/sp1ff/mpdpopm/releases/download/0.3.0/mpdpopm-0.3.0.tar.xz -tar xf mpdpopm-0.3.0.tar.xz -cd mpdpopm-0.3.0 +curl -L -O https://github.com/sp1ff/mpdpopm/releases/download/0.3.1/mpdpopm-0.3.1.tar.xz +tar xf mpdpopm-0.3.1.tar.xz +cd mpdpopm-0.3.1 ./configure make make check diff --git a/admin/Dockerfile-arch b/admin/Dockerfile-arch index dad4d55..c47552d 100644 --- a/admin/Dockerfile-arch +++ b/admin/Dockerfile-arch @@ -8,32 +8,32 @@ RUN pacman --noconfirm -Syu && \ cp -v /tmp/pacman.conf.tmp /etc/pacman.conf # WORKAROUND for glibc 2.33 and old Docker +# Seems to be no longer needed # See https://github.com/actions/virtual-environments/issues/2658 # Thanks to https://github.com/lxqt/lxqt-panel/pull/1562 -RUN patched_glibc=glibc-linux4-2.33-4-x86_64.pkg.tar.zst && \ - curl -LO "https://repo.archlinuxcn.org/x86_64/$patched_glibc" && \ - bsdtar -C / -xvf "$patched_glibc" +# RUN patched_glibc=glibc-linux4-2.33-4-x86_64.pkg.tar.zst && \ +# curl -LO "https://repo.archlinuxcn.org/x86_64/$patched_glibc" && \ +# bsdtar -C / -xvf "$patched_glibc" # Get the mirrorlist up-to-date -RUN pacman rsync --noconfirm -S reflector rsync && reflector --latest 16 --protocol https --sort rate --save /etc/pacman.d/mirrorlist +# RUN pacman rsync --noconfirm -S reflector rsync && \ +# reflector --latest 16 --protocol https --sort rate --save /etc/pacman.d/mirrorlist -# TODO(sp1ff): WTF? See here https://bbs.archlinux.org/viewtopic.php?id=141029 -# RUN pacman --noconfirm -S man-db man-pages texinfo vim base-devel gdb && \ -RUN pacman --noconfirm -Syyu man-db man-pages texinfo vim base-devel gdb rust cargo && \ +# See here https://bbs.archlinux.org/viewtopic.php?id=141029 +# Not sure why the -yyu needed, perhaps as a result of reflector changing the mirrorlist? +# RUN pacman --noconfirm -Syyu man-db man-pages texinfo vim base-devel gdb rust cargo && \ +RUN pacman --noconfirm -S man-db man-pages texinfo vim base-devel gdb rust cargo && \ ln -sf /usr/share/zoneinfo/America/Los_Angeles /etc/localtime && \ useradd -ms /bin/bash -G users,wheel mgh && \ echo 'mgh:mgh' | chpasswd && \ echo "%wheel ALL=(ALL) NOPASSWD: ALL" >> /etc/sudoers && \ mkdir /cores && chmod 777 /cores && \ - echo "kernel.core_pattern=/cores/core.%e.%p" >> /etc/sysctl.d/50-coredump.conf + echo "kernel.core_pattern=/cores/core.%e.%p" >> /etc/sysctl.d/50-coredump.conf -# TODO(sp1ff): needed? -# sysctl -p /etc/sysctl.d/50-coredump.conf -# RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | bash -s -- -y +# Rhm... needed? +# sudo sysctl -p /etc/sysctl.d/50-coredump.conf USER mgh -# RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > rust.sh && chmod 755 ./rust.sh && ./rust.sh -y - ENV PATH="/home/mgh/.cargo/bin:${PATH}" diff --git a/admin/Dockerfile-debian b/admin/Dockerfile-debian index b3cf58d..abac88f 100644 --- a/admin/Dockerfile-debian +++ b/admin/Dockerfile-debian @@ -30,7 +30,9 @@ RUN set -ex && \ echo "path-include /usr/share/man/man1/scribbu*" >> /etc/dpkg/dpkg.cfg.d/docker && \ echo "path-include /usr/share/info/scribbu.info" >> /etc/dpkg/dpkg.cfg.d/docker && \ useradd -ms /bin/bash -G users,sudo mgh && \ - echo 'mgh:mgh' | chpasswd + echo 'mgh:mgh' | chpasswd && \ + cp -v /etc/sudoers /etc/sudoers.orig && \ + echo 'mgh ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers # The test suite won't work if this isn't set: ENV LANG="en_US.UTF-8" diff --git a/admin/build-dev-arch-pkg-cnt b/admin/build-dev-arch-pkg-cnt index 348a59a..970d5c5 100755 --- a/admin/build-dev-arch-pkg-cnt +++ b/admin/build-dev-arch-pkg-cnt @@ -8,4 +8,9 @@ cp -v /PKGBUILD /tmp cd /tmp makepkg -g >> PKGBUILD makepkg +sudo pacman --noconfirm -Uv mpdpopm-${version}-1-x86_64.pkg.tar.zst +mppopm --version +mppopm --help +test -f /usr/local/share/info/mpdpopm.info.gz +sudo pacman --noconfirm -R mpdpopm cp -v mpdpopm-${version}-1-x86_64.pkg.tar.zst /mpdpopm diff --git a/admin/build-dev-debian-pkg-cnt b/admin/build-dev-debian-pkg-cnt index 0d14c91..60d16a5 100755 --- a/admin/build-dev-debian-pkg-cnt +++ b/admin/build-dev-debian-pkg-cnt @@ -11,5 +11,10 @@ cp -v /LICENSE . ./configure cd mpdpopm cargo deb +sudo dpkg --debug=2 -i target/debian/mpdpopm_${version}_amd64.deb +mppopm --version +mppopm --help +test -f /usr/local/share/info/mpdpopm.info +sudo dpkg --debug=2 -r mpdpopm sudo cp -v target/debian/mpdpopm_${version}_amd64.deb /mpdpopm diff --git a/configure.ac b/configure.ac index 572bbfe..4caa001 100644 --- a/configure.ac +++ b/configure.ac @@ -1,4 +1,4 @@ -AC_INIT([mpdpopm], [0.3.0], [sp1ff@pobox.com], [mpdpopm], [https://github.com/sp1ff/mpdpopm]) +AC_INIT([mpdpopm], [0.3.1], [sp1ff@pobox.com], [mpdpopm], [https://github.com/sp1ff/mpdpopm]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_SRCDIR([mpdpopm/Cargo.toml.in]) AM_INIT_AUTOMAKE([-Wall -Werror -Wno-portability -Wno-override gnits std-options dist-bzip2 dist-xz]) diff --git a/doc/version.texi b/doc/version.texi index a50f21b..4fb424b 100644 --- a/doc/version.texi +++ b/doc/version.texi @@ -1,4 +1,4 @@ @set UPDATED 29 March 2021 @set UPDATED-MONTH March 2021 -@set EDITION 0.3.0 -@set VERSION 0.3.0 +@set EDITION 0.3.1 +@set VERSION 0.3.1 diff --git a/mpdpopm/Cargo.toml.in b/mpdpopm/Cargo.toml.in index 1b3564c..da2f707 100644 --- a/mpdpopm/Cargo.toml.in +++ b/mpdpopm/Cargo.toml.in @@ -17,21 +17,22 @@ lalrpop = { version = "0.19", features = ["lexer"] } [dependencies] async-trait = "0.1.31" +backtrace = "0.3.46" boolinator = "2.4.0" chrono = "0.4.19" clap = "=3.0.0-beta.1" errno = "0.2.6" futures = "0.3.5" lalrpop-util = "0.19" +lazy_static = "1.4.0" libc = "0.2.74" log = "0.4.8" log4rs = "0.10.0" os_str_bytes = "2.3.1" -pin-project = "0.4.17" +pin-project = "1.0.8" regex = "1.3.6" serde = { version = "1.0", features = ["derive"] } serde-lexpr = "0.1.1" -snafu = { version = "0.6.7", features = ["backtraces"] } tokio = { version = "0.2.22", features = ["dns", "io-util", "macros", "process", "rt-threaded", "signal", "tcp", "time", "uds"] } [package.metadata.deb] diff --git a/mpdpopm/Makefile.am b/mpdpopm/Makefile.am index 1e751e5..d28d3e0 100644 --- a/mpdpopm/Makefile.am +++ b/mpdpopm/Makefile.am @@ -39,7 +39,6 @@ DISTCLEANFILES = $(builddir)/Cargo.toml common_sources = clients.rs \ commands.rs \ config.rs \ - error_from.rs \ lib.rs \ messages.rs \ playcounts.rs \ diff --git a/mpdpopm/src/bin/mppopm.rs b/mpdpopm/src/bin/mppopm.rs index 5949297..ace52d3 100644 --- a/mpdpopm/src/bin/mppopm.rs +++ b/mpdpopm/src/bin/mppopm.rs @@ -28,11 +28,11 @@ use mpdpopm::{ clients::{quote, Client, PlayerStatus}, - error_from, playcounts::{get_last_played, get_play_count}, ratings::get_rating, }; +use backtrace::Backtrace; use clap::{App, Arg}; use log::{debug, info, trace, LevelFilter}; use log4rs::{ @@ -41,7 +41,6 @@ use log4rs::{ encode::pattern::PatternEncoder, }; use serde::{Deserialize, Serialize}; -use snafu::{Backtrace, GenerateBacktrace, OptionExt, Snafu}; use std::{fmt, path::PathBuf}; @@ -49,94 +48,79 @@ use std::{fmt, path::PathBuf}; // mppopm application Error Type // //////////////////////////////////////////////////////////////////////////////////////////////////// -// NB we take care NOT to derive Debug here. This is because main returns a Result<(), Error>; in -// the case of an error, the stdlib will format the resulting error message using the Debug trait -// which, when derived, is rather ugly. We'll implement it by hand below to produce something more -// pleasant for human beings to read. -/// `mppopm` errors -#[derive(Snafu)] +#[non_exhaustive] pub enum Error { - #[snafu(display("{}", cause))] - Other { - #[snafu(source(true))] - cause: Box, - #[snafu(backtrace(true))] - back: Backtrace, - }, - #[snafu(display("No sub-command specified; try `mppopm --help'"))] NoSubCommand, - #[snafu(display( - "The `config' argument couldn't be retrieved. This is likely a bug; please \ -consider filing a report with sp1ff@pobox.com" - ))] NoConfigArg, - #[snafu(display( - "The `rating' argument couldn't be retrieved. This is likely a bug; please \ -consider filing a report with sp1ff@pobox.com" - ))] NoRating, - #[snafu(display( - "The `playcount' argument couldn't be retrieved. This is likely a bug; please \ -consider filing a report with sp1ff@pobox.com" - ))] NoPlayCount, - #[snafu(display( - "The `last-played' argument couldn't be retrieved. This is likely a bug; please \ -consider filing a report with sp1ff@pobox.com" - ))] NoLastPlayed, - #[snafu(display( - "While trying to read the configuration file `{:?}', got `{}'", - config, - cause - ))] NoConfig { config: std::path::PathBuf, - #[snafu(source(true))] cause: std::io::Error, }, - #[snafu(display("Can't retrieve the current song when the player is stopped"))] PlayerStopped, - #[snafu(display("Received a path with non-UTF8 codepoints: {:?}", path))] BadPath { path: PathBuf, - #[snafu(backtrace(true))] back: Backtrace, }, - #[cfg(feature = "scribbu")] - #[snafu(display( - "The `xtag' argument couldn't be retrieved. This is likely a bug; please \ -consider filing a report with sp1ff@pobox.com" - ))] - NoXtag, - #[cfg(feature = "scribbu")] - #[snafu(display( - "The `genre' argument couldn't be retrieved. This is likely a bug; please \ -consider filing a report with sp1ff@pobox.com" - ))] - NoGenre, - #[snafu(display( - "The `playlist' argument couldn't be retrieved. This is likely a bug; \ -please consider filing a report with sp1ff@pobox.com" - ))] NoPlaylist, + Client { + source: mpdpopm::clients::Error, + back: Backtrace, + }, + Ratings { + source: mpdpopm::ratings::Error, + back: Backtrace, + }, + Playcounts { + source: mpdpopm::playcounts::Error, + back: Backtrace, + }, + ExpectedInt { + source: std::num::ParseIntError, + back: Backtrace, + }, + Logging { + source: log::SetLoggerError, + back: Backtrace, + }, + Config { + source: serde_lexpr::Error, + back: Backtrace, + }, } +impl std::fmt::Display for Error { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::NoSubCommand => write!(f, "No sub-command given"), + Error::NoConfigArg => write!(f, "No argument given for the configuration option"), + Error::NoRating => write!(f, "No rating supplied"), + Error::NoPlayCount => write!(f, "No play count supplied"), + Error::NoLastPlayed => write!(f, "No last played timestamp given"), + Error::NoConfig { config, cause } => write!(f, "Bad config ({:?}): {}", config, cause), + Error::PlayerStopped => write!(f, "The player is stopped"), + Error::BadPath { path, back: _ } => write!(f, "Bad path: {:?}", path), + Error::NoPlaylist => write!(f, "No playlist given"), + Error::Client { source, back: _ } => write!(f, "Client error: {}", source), + Error::Ratings { source, back: _ } => write!(f, "Rating error: {}", source), + Error::Playcounts { source, back: _ } => write!(f, "Playcount error: {}", source), + Error::ExpectedInt { source, back: _ } => write!(f, "Expected integer: {}", source), + Error::Logging { source, back: _ } => write!(f, "Logging error: {}", source), + Error::Config { source, back: _ } => { + write!(f, "Error reading configuration: {}", source) + } + } + } +} impl fmt::Debug for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{}", self) } } -error_from!(log::SetLoggerError); -error_from!(mpdpopm::Error); -error_from!(mpdpopm::clients::Error); -error_from!(mpdpopm::playcounts::Error); -error_from!(mpdpopm::ratings::Error); -error_from!(serde_lexpr::error::Error); -error_from!(std::env::VarError); -error_from!(std::num::ParseIntError); - type Result = std::result::Result; //////////////////////////////////////////////////////////////////////////////////////////////////// @@ -159,7 +143,7 @@ pub struct Config { /// Channel to setup for assorted commands-- channel names must satisfy "[-a-zA-Z-9_.:]+" commands_chan: String, /// Sticker under which to store song ratings, as a textual representation of a number in - /// [0,255] + /// `[0,255]` rating_sticker: String, } @@ -195,12 +179,16 @@ async fn map_tracks<'a, Iter: Iterator>( let files = match args { Some(iter) => iter.map(|x| x.to_string()).collect(), None => { - let file = match client.status().await? { + let file = match client.status().await.map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? { PlayerStatus::Play(curr) | PlayerStatus::Pause(curr) => curr .file .to_str() - .context(BadPath { + .ok_or_else(|| Error::BadPath { path: curr.file.clone(), + back: Backtrace::new(), })? .to_string(), PlayerStatus::Stopped => { @@ -226,7 +214,12 @@ async fn get_ratings<'a, Iter: Iterator>( ) -> Result<()> { let mut ratings: Vec<(String, u8)> = Vec::new(); for file in map_tracks(client, tracks).await? { - let rating = get_rating(client, sticker, &file).await?; + let rating = get_rating(client, sticker, &file) + .await + .map_err(|err| Error::Ratings { + source: err, + back: Backtrace::new(), + })?; ratings.push((file, rating)); } @@ -252,7 +245,13 @@ async fn set_rating( Some(uri) => format!("rate {} \\\"{}\\\"", rating, uri), None => format!("rate {}", rating), }; - client.send_message(chan, &cmd).await?; + client + .send_message(chan, &cmd) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; match arg { Some(uri) => info!("Set the rating for \"{}\" to \"{}\".", uri, rating), @@ -271,7 +270,12 @@ async fn get_play_counts<'a, Iter: Iterator>( ) -> Result<()> { let mut playcounts: Vec<(String, usize)> = Vec::new(); for file in map_tracks(client, tracks).await? { - let playcount = match get_play_count(client, sticker, &file).await? { + let playcount = match get_play_count(client, sticker, &file) + .await + .map_err(|err| Error::Playcounts { + source: err, + back: Backtrace::new(), + })? { Some(pc) => pc, None => 0, }; @@ -300,7 +304,13 @@ async fn set_play_counts( Some(uri) => format!("setpc {} \\\"{}\\\"", playcount, uri), None => format!("setpc {}", playcount), }; - client.send_message(chan, &cmd).await?; + client + .send_message(chan, &cmd) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; match arg { Some(uri) => info!("Set the playcount for \"{}\" to \"{}\".", uri, playcount), @@ -322,7 +332,12 @@ async fn get_last_playeds<'a, Iter: Iterator>( ) -> Result<()> { let mut lastplayeds: Vec<(String, Option)> = Vec::new(); for file in map_tracks(client, tracks).await? { - let lastplayed = get_last_played(client, sticker, &file).await?; + let lastplayed = get_last_played(client, sticker, &file) + .await + .map_err(|err| Error::Playcounts { + source: err, + back: Backtrace::new(), + })?; lastplayeds.push((file, lastplayed)); } @@ -361,7 +376,13 @@ async fn set_last_playeds( Some(uri) => format!("setlp {} {}", lastplayed, uri), None => format!("setlp {}", lastplayed), }; - client.send_message(chan, &cmd).await?; + client + .send_message(chan, &cmd) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; match arg { Some(uri) => info!("Set last played for \"{}\" to \"{}\".", uri, lastplayed), @@ -376,7 +397,13 @@ async fn set_last_playeds( /// Retrieve the list of stored playlists async fn get_playlists(client: &mut Client) -> Result<()> { - let mut pls = client.get_stored_playlists().await?; + let mut pls = client + .get_stored_playlists() + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; pls.sort(); println!("Stored playlists:"); for pl in pls { @@ -390,7 +417,13 @@ async fn findadd(client: &mut Client, chan: &str, filter: &str, case: bool) -> R let qfilter = quote(filter); debug!("findadd: got ``{}'', quoted to ``{}''.", filter, qfilter); let cmd = format!("{} {}", if case { "findadd" } else { "searchadd" }, qfilter); - client.send_message(chan, &cmd).await?; + client + .send_message(chan, &cmd) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; Ok(()) } @@ -407,7 +440,11 @@ where args.map(|a| quote(a)).collect::>().join(" ") ), ) - .await?; + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; Ok(()) } @@ -591,7 +628,7 @@ will add all songs whose artist tag matches the regexp \"pogues\" with a rating async fn main() -> Result<()> { use mpdpopm::vars::{AUTHOR, VERSION}; - let def_cfg = format!("{}/.mppopm", std::env::var("HOME")?); + let def_cfg = format!("{}/.mppopm", std::env::var("HOME").unwrap()); let mut app = App::new("mppopm") .version(VERSION) .author(AUTHOR) @@ -640,11 +677,16 @@ async fn main() -> Result<()> { // and it's not there, that's fine: we just proceed with a defualt configuration. But if they // explicitly named a configuration file, and it's not there, they presumably want to know // about that. - let cfgpth = matches.value_of("config").context(NoConfigArg {})?; + let cfgpth = matches + .value_of("config") + .ok_or_else(|| Error::NoConfigArg {})?; let mut cfg = match std::fs::read_to_string(cfgpth) { // The config file (defaulted or not) existed & we were able to read its contents-- parse // em! - Ok(text) => serde_lexpr::from_str(&text)?, + Ok(text) => serde_lexpr::from_str(&text).map_err(|err| Error::Config { + source: err, + back: Backtrace::new(), + })?, // The config file (defaulted or not) either didn't exist, or we were unable to read its // contents... Err(err) => match (err.kind(), matches.occurrences_of("config")) { @@ -687,12 +729,18 @@ async fn main() -> Result<()> { match matches.value_of("port") { Some(port) => { - cfg.port = port.parse::()?; + cfg.port = port.parse::().map_err(|err| Error::ExpectedInt { + source: err, + back: Backtrace::new(), + })?; } None => { if cfg.port == 0 { cfg.port = match std::env::var("MPD_PORT") { - Ok(port) => port.parse::()?, + Ok(port) => port.parse::().map_err(|err| Error::ExpectedInt { + source: err, + back: Backtrace::new(), + })?, Err(_) => 6600, } } @@ -714,12 +762,20 @@ async fn main() -> Result<()> { .appender(Appender::builder().build("stdout", Box::new(app))) .build(Root::builder().appender("stdout").build(lf)) .unwrap(); - log4rs::init_config(lcfg)?; + log4rs::init_config(lcfg).map_err(|err| Error::Logging { + source: err, + back: Backtrace::new(), + })?; trace!("logging configured."); // Whatever we do, we're going to need a Client, so whip one up now: - let mut client = Client::connect(format!("{}:{}", cfg.host, cfg.port)).await?; + let mut client = Client::connect(format!("{}:{}", cfg.host, cfg.port)) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; if let Some(subm) = matches.subcommand_matches("get-rating") { return Ok(get_ratings( @@ -733,7 +789,7 @@ async fn main() -> Result<()> { return Ok(set_rating( &mut client, &cfg.commands_chan, - subm.value_of("rating").context(NoRating {})?, + subm.value_of("rating").ok_or_else(|| Error::NoRating {})?, subm.value_of("track"), ) .await?); @@ -750,8 +806,12 @@ async fn main() -> Result<()> { &mut client, &cfg.commands_chan, subm.value_of("play-count") - .context(NoPlayCount {})? - .parse::()?, + .ok_or_else(|| Error::NoPlayCount {})? + .parse::() + .map_err(|err| Error::ExpectedInt { + source: err, + back: Backtrace::new(), + })?, subm.value_of("track"), ) .await?); @@ -768,8 +828,12 @@ async fn main() -> Result<()> { &mut client, &cfg.commands_chan, subm.value_of("last-played") - .context(NoLastPlayed {})? - .parse::()?, + .ok_or_else(|| Error::NoLastPlayed {})? + .parse::() + .map_err(|err| Error::ExpectedInt { + source: err, + back: Backtrace::new(), + })?, subm.value_of("track"), ) .await?); diff --git a/mpdpopm/src/bin/mppopmd.rs b/mpdpopm/src/bin/mppopmd.rs index b2fb17e..288983b 100644 --- a/mpdpopm/src/bin/mppopmd.rs +++ b/mpdpopm/src/bin/mppopmd.rs @@ -27,10 +27,10 @@ use mpdpopm::config; use mpdpopm::config::Config; -use mpdpopm::error_from; use mpdpopm::mpdpopm; use mpdpopm::vars::LOCALSTATEDIR; +use backtrace::Backtrace; use clap::{App, Arg}; use errno::errno; use libc::{ @@ -45,7 +45,6 @@ use log4rs::{ config::{Appender, Root}, encode::pattern::PatternEncoder, }; -use snafu::{Backtrace, GenerateBacktrace, OptionExt, Snafu}; use std::{ffi::CString, fmt, path::PathBuf}; @@ -53,111 +52,105 @@ use std::{ffi::CString, fmt, path::PathBuf}; // mppopmd application Error type // //////////////////////////////////////////////////////////////////////////////////////////////////// -// Anything that implements std::process::Termination can be used as the return type for `main'. The -// std lib implements impl Termination for Result<()undefined E> per -// , so we're good-- just don't -// derive Debug-- implement by hand to produce something human-friendly (since this will be used for -// main's return value) -#[derive(Snafu)] +#[non_exhaustive] pub enum Error { - #[snafu(display("{}", cause))] - Other { - #[snafu(source(true))] - cause: Box, - #[snafu(backtrace(true))] - back: Backtrace, - }, - #[snafu(display( - "The config argument couldn't be retrieved. This is likely a bug; please \ -consider filing a report with sp1ff@pobox.com" - ))] NoConfigArg, - #[snafu(display( - "While trying to read the configuration file `{:?}', got `{}'", - config, - cause - ))] NoConfig { config: std::path::PathBuf, - #[snafu(source(true))] cause: std::io::Error, }, - #[snafu(display("Failed to fork this process: {}", errno))] Fork { errno: errno::Errno, - #[snafu(backtrace(true))] - back: Backtrace, - }, - #[snafu(display("Failed to open /dev/null: {}", errno))] - DevNull { - errno: errno::Errno, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("Path to lock file contained a null"))] PathContainsNull { - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("Error opening lock file: {}", errno))] OpenLockFile { errno: errno::Errno, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("Error locking lockfile: {}", errno))] LockFile { errno: errno::Errno, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("Error writing pid to lockfile: {}", errno))] WritePid { errno: errno::Errno, - #[snafu(backtrace(true))] + back: Backtrace, + }, + Config { + source: crate::config::Error, + back: Backtrace, + }, + Logging { + source: log::SetLoggerError, + back: Backtrace, + }, + MpdPopm { + source: mpdpopm::Error, back: Backtrace, }, } +impl std::fmt::Display for Error { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::NoConfigArg => write!(f, "No configuration file given"), + Error::NoConfig { config, cause } => { + write!(f, "Configuration error ({:?}): {}", config, cause) + } + Error::Fork { errno, back: _ } => write!(f, "When forking, got errno {}", errno), + Error::PathContainsNull { back: _ } => write!(f, "Path contains a null character"), + Error::OpenLockFile { errno, back: _ } => { + write!(f, "While opening lock file, got errno {}", errno) + } + Error::LockFile { errno, back: _ } => { + write!(f, "While locking the lock file, got errno {}", errno) + } + Error::WritePid { errno, back: _ } => { + write!(f, "While writing pid file, got errno {}", errno) + } + Error::Config { source, back: _ } => write!(f, "Configuration error: {}", source), + Error::Logging { source, back: _ } => write!(f, "Logging error: {}", source), + Error::MpdPopm { source, back: _ } => write!(f, "mpdpopm error: {}", source), + _ => write!(f, "Unknown mppopmd error"), + } + } +} + impl fmt::Debug for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{}", self) } } -error_from!(log::SetLoggerError); -error_from!(mpdpopm::Error); -error_from!(mpdpopm::config::Error); -error_from!(serde_lexpr::error::Error); -error_from!(std::ffi::NulError); -error_from!(std::io::Error); - type Result = std::result::Result<(), Error>; /// Make this process into a daemon /// -/// I first tried to use the `daemonize' crate, without success. Perhaps I wasn't using it -/// correctly. In any event, both to debug the issue(s) and for my own edification, I decided to -/// hand-code the daemonization process. +/// I first tried to use the [daemonize](https://docs.rs/daemonize) crate, without success. Perhaps +/// I wasn't using it correctly. In any event, both to debug the issue(s) and for my own +/// edification, I decided to hand-code the daemonization process. /// /// After spending a bit of time digging around the world of TTYs, process groups & sessions, I'm /// beginning to understand what "daemon" means and how to create one. The first step is to /// dissassociate this process from it's controlling terminal & make sure it cannot acquire a new -/// one. This, AFAIK, is to disconnect us from any job control associated with that terminal, and in +/// one. This, AFAIU, is to disconnect us from any job control associated with that terminal, and in /// particular to prevent us from being disturbed when & if that terminal is closed (I'm still hazy /// on the details, but at least the session leader (and perhaps it's descendants) will be sent a -/// SIGHUP in that eventuality). +/// `SIGHUP` in that eventuality). /// /// After that, the rest of the work seems to consist of shedding all the things we (may have) /// inherited from our creator. Things such as: /// -/// - pwd -/// - umask -/// - all file descriptors -/// - stdin, stdout & stderr should be closed (we don't know from or to where they may have -/// been redirected), and re-opened to locations appropriate to this daemon -/// - any other file descriptors should be closed; this process can then re-open any that -/// it needs for its work +/// - pwd +/// - umask +/// - all file descriptors +/// - stdin, stdout & stderr should be closed (we don't know from or to where they may have +/// been redirected), and re-opened to locations appropriate to this daemon +/// - any other file descriptors should be closed; this process can then re-open any that +/// it needs for its work /// /// In the end, the problem turned out not to be the daeomonize crate, but rather a more subtle /// issue with the interaction between forking & the tokio runtime-- tokio will spin-up a thread @@ -165,9 +158,11 @@ type Result = std::result::Result<(), Error>; /// starting-up the tokio runtime. In any event, I learned a lot about process management & wound up /// choosing to do a few things differently. /// -/// -/// -/// +/// References: +/// +/// - +/// - +/// - fn daemonize() -> Result { use std::os::unix::ffi::OsStringExt; @@ -182,7 +177,7 @@ fn daemonize() -> Result { if pid < 0 { return Err(Error::Fork { errno: errno(), - back: Backtrace::generate(), + back: Backtrace::new(), }); } else if pid != 0 { // We are the parent process-- exit. @@ -202,7 +197,7 @@ fn daemonize() -> Result { if pid < 0 { return Err(Error::Fork { errno: errno(), - back: Backtrace::generate(), + back: Backtrace::new(), }); } else if pid != 0 { // We are the parent process-- exit. @@ -211,7 +206,7 @@ fn daemonize() -> Result { // We next change the present working directory to avoid keeping the present one in // use. `mppopmd' can run pretty much anywhere, so /tmp is as good a place as any. - std::env::set_current_dir("/tmp")?; + std::env::set_current_dir("/tmp").unwrap(); umask(0); @@ -230,7 +225,7 @@ fn daemonize() -> Result { dup(i); let pth: PathBuf = [LOCALSTATEDIR, "run", "mppopmd.pid"].iter().collect(); - let pth_c = CString::new(pth.into_os_string().into_vec())?; + let pth_c = CString::new(pth.into_os_string().into_vec()).unwrap(); let fd = open( pth_c.as_ptr(), libc::O_RDWR | libc::O_CREAT | libc::O_TRUNC, @@ -239,13 +234,13 @@ fn daemonize() -> Result { if -1 == fd { return Err(Error::OpenLockFile { errno: errno(), - back: Backtrace::generate(), + back: Backtrace::new(), }); } if lockf(fd, F_TLOCK, 0) < 0 { return Err(Error::LockFile { errno: errno(), - back: Backtrace::generate(), + back: Backtrace::new(), }); } @@ -255,11 +250,11 @@ fn daemonize() -> Result { let pid = getpid(); let pid_buf = format!("{}", pid).into_bytes(); let pid_length = pid_buf.len(); - let pid_c = CString::new(pid_buf)?; + let pid_c = CString::new(pid_buf).unwrap(); if write(fd, pid_c.as_ptr() as *const libc::c_void, pid_length) < pid_length as isize { return Err(Error::WritePid { errno: errno(), - back: Backtrace::generate(), + back: Backtrace::new(), }); } } @@ -320,11 +315,16 @@ commands to keep your tags up-to-date.", // and it's not there, that's fine: we just proceed with a default configuration values. But if // they explicitly named a configuration file, and it's not there, they presumably want to know // about that. - let cfgpth = matches.value_of("config").context(NoConfigArg {})?; + let cfgpth = matches + .value_of("config") + .ok_or_else(|| Error::NoConfigArg {})?; let cfg = match std::fs::read_to_string(cfgpth) { // The config file (defaulted or not) existed & we were able to read its contents-- parse // em! - Ok(text) => config::from_str(&text)?, + Ok(text) => config::from_str(&text).map_err(|err| Error::Config { + source: err, + back: Backtrace::new(), + })?, // The config file (defaulted or not) either didn't exist, or we were unable to read its // contents... Err(err) => match (err.kind(), matches.occurrences_of("config")) { @@ -364,7 +364,10 @@ commands to keep your tags up-to-date.", .appender(Appender::builder().build("stdout", Box::new(app))) .build(Root::builder().appender("stdout").build(lf)) .unwrap(); - log4rs::init_config(lcfg)?; + log4rs::init_config(lcfg).map_err(|err| Error::Logging { + source: err, + back: Backtrace::new(), + })?; } else { // Else, daemonize this process now, before we spin-up the Tokio runtime. daemonize()?; @@ -377,17 +380,19 @@ commands to keep your tags up-to-date.", .appender(Appender::builder().build("logfile", Box::new(app))) .build(Root::builder().appender("logfile").build(lf)) .unwrap(); - log4rs::init_config(lcfg)?; + log4rs::init_config(lcfg).map_err(|err| Error::Logging { + source: err, + back: Backtrace::new(), + })?; } // One way or another, we are now the "final" process. Announce ourselves... info!("mppopmd {} logging at level {:#?}.", VERSION, lf); // spin-up the Tokio runtime... let mut rt = tokio::runtime::Runtime::new().unwrap(); - // and invoke `mpdpopm'. I can't figure out how to get Rust to map from Result<(), - // mpdpopm::Error> to Result<(), Error>, so I'm stuck with this ugly match statement: - match rt.block_on(mpdpopm(cfg)) { - Ok(_) => Ok(()), - Err(err) => Err(Error::from(err)), - } + // and invoke `mpdpopm'. + rt.block_on(mpdpopm(cfg)).map_err(|err| Error::MpdPopm { + source: err, + back: Backtrace::new(), + }) } diff --git a/mpdpopm/src/clients.rs b/mpdpopm/src/clients.rs index be3be88..1e7e2e1 100644 --- a/mpdpopm/src/clients.rs +++ b/mpdpopm/src/clients.rs @@ -13,34 +13,35 @@ // You should have received a copy of the GNU General Public License along with mpdpopm. If not, // see . -//! # mpd clients and associated utilities. +//! mpd clients and associated utilities. //! -//! ## Introduction +//! # Introduction //! -//! This module contains basic types implementing various `mpd' client operations. Cf. the [mpd -//! protocol](http://www.musicpd.org/doc/protocol/). Since issuing the "idle" command will tie up -//! the connection, mpd clients often use multiple connections to the server (one to listen for +//! This module contains basic types implementing various MPD client operations (cf. the [mpd +//! protocol](http://www.musicpd.org/doc/protocol/)). Since issuing the "idle" command will tie up +//! the connection, MPD clients often use multiple connections to the server (one to listen for //! updates, one or more on which to issue commands). This modules provides two different client -//! types: [`Client`] for general-purpose use and [`IdleClient`] for long-lived connections -//! listening for server notifiations. +//! types: [Client] for general-purpose use and [IdleClient] for long-lived connections listening +//! for server notifiations. //! -//! There *is* another idiom (used in libmpdel, e.g.): open a single connection & issue an "idle" -//! command. When you want to issue a command, send a "noidle", then the command, then "idle" again. -//! This isn't a race condition, as the server will buffer any changes that took place when you -//! were not idle & send them when you re-issue the "idle" command. - -use crate::error_from; +//! Note that there *is* another idiom (used in [libmpdel](https://github.com/mpdel/libmpdel), +//! e.g.): open a single connection & issue an "idle" command. When you want to issue a command, +//! send a "noidle", then the command, then "idle" again. This isn't a race condition, as the +//! server will buffer any changes that took place when you were not idle & send them when you +//! re-issue the "idle" command. This crate however takes the approach of two channels (like +//! [mpdfav](https://github.com/vincent-petithory/mpdfav)). use async_trait::async_trait; -use boolinator::Boolinator; +use backtrace::Backtrace; use log::{debug, info}; use regex::Regex; -use snafu::{Backtrace, GenerateBacktrace, OptionExt, Snafu}; use tokio::{ net::{TcpStream, ToSocketAddrs, UnixStream}, prelude::*, }; +use lazy_static::lazy_static; + use std::{ collections::HashMap, convert::TryFrom, @@ -54,176 +55,194 @@ use std::{ // Error // //////////////////////////////////////////////////////////////////////////////////////////////////// -/// An enumeration of `mpd' client errors -#[derive(Debug, Snafu)] +// The Protocol error, below, gets used a *lot*; anywhere we receive a message from the MPD server +// that "should" never happen. To help give a bit of context beyond a stack trace, I use this +// enumeration of "operations" +/// Enumerated list of MPD operations; used in Error::Protocol to distinguish which operation it was +/// that elicited the protocol error. +#[derive(Debug)] +#[non_exhaustive] +pub enum Operation { + Connect, + Status, + GetSticker, + SetSticker, + SendToPlaylist, + SendMessage, + Update, + GetStoredPlaylists, + RspToUris, + GetStickers, + GetAllSongs, + Add, + Idle, + GetMessages, +} + +impl std::fmt::Display for Operation { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Operation::Connect => write!(f, "Connect"), + Operation::Status => write!(f, "Status"), + Operation::GetSticker => write!(f, "GetSticker"), + Operation::SetSticker => write!(f, "SetSticker"), + Operation::SendToPlaylist => write!(f, "SendToPlaylist"), + Operation::SendMessage => write!(f, "SendMessage"), + Operation::Update => write!(f, "Update"), + Operation::GetStoredPlaylists => write!(f, "GetStoredPlaylists"), + Operation::RspToUris => write!(f, "RspToUris"), + Operation::GetStickers => write!(f, "GetStickers"), + Operation::GetAllSongs => write!(f, "GetAllSongs"), + Operation::Add => write!(f, "Add"), + Operation::Idle => write!(f, "Idle"), + Operation::GetMessages => write!(f, "GetMessages"), + _ => write!(f, "Unknown client operation"), + } + } +} + +/// An MPD client error +#[derive(Debug)] +#[non_exhaustive] pub enum Error { - #[snafu(display("{}", cause))] - Other { - #[snafu(source(true))] - cause: Box, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error adding a URI - #[snafu(display("Failed to add URI: `{}'", text))] - Add { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error upon connecting to the mpd server; includes the text returned (if any) - #[snafu(display("Failed to connect to the mpd server: `{}'", text))] - Connect { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in response to find1 or find2 - #[snafu(display("Search failed: {}", text))] - Find { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, + Protocol { + op: Operation, + msg: String, + back: backtrace::Backtrace, }, - /// Error in response to get_all_songs - #[snafu(display("Failed to fetch all song URIs: {}", text))] - GetAllSongs { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, + Io { + source: std::io::Error, + back: backtrace::Backtrace, }, - /// Error in respone to "readmessages" - #[snafu(display("Failed to read messages: `{}'", text))] - GetMessages { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in response to "sticker find" - #[snafu(display("Failed to find stickers: `{}'", text))] - GetStickers { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in respone to "sticker set" - #[snafu(display("Failed to set sticker: `{}'", text))] - StickerSet { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in respone to "sticker get" - #[snafu(display("Failed to get sticker: `{}'", text))] - StickerGet { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in respone to "sendmessage" - #[snafu(display("Failed to send message: `{}'", text))] - SendMessage { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in respone to "subscribe" - #[snafu(display("Failed to subscribe to our subsystems of interest: `{}'", text))] - Subscribe { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in respone to "idle" - #[snafu(display("Failed to idle: `{}'", text))] - Idle { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Unknown idle subsystem - #[snafu(display( - "Uknown subsystem `{}'returned in respones to the `idle' command. This is \ -likely a bug; please consider reporting it to sp1ff@pobox.com", - text - ))] - IdleSubsystem { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, + Encoding { + source: std::string::FromUtf8Error, + back: backtrace::Backtrace, }, - /// Couldn't parse PlayState - #[snafu(display("Couldn't parse play state from {}", text))] - PlayState { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Couldn't parse SongId - #[snafu(display("Couldn't parse songid from {}", text))] - SongId { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Couldn't parse Elapsed - #[snafu(display("Couldn't parse elapsed from {}", text))] - Elapsed { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Couldn't parse song file - #[snafu(display("Couldn't parse song file from {}", text))] - SongFile { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Couldn't parse Duration - #[snafu(display("Couldn't parse elapsed from {}", text))] - Duration { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Unknown player state - #[snafu(display("Unknown player state {}", state))] - UnknownState { - state: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in respone to "update" - #[snafu(display("Failed to update DB: `{}'", text))] - UpdateDB { - text: String, - #[snafu(backtrace(true))] - back: Backtrace, + StickerConversion { + sticker: String, + source: Box, + back: backtrace::Backtrace, }, - /// Error in respone to "listplaylists" - #[snafu(display("Failed to list stored playlists: `{}'", text))] - GetStoredPlaylists { + IdleSubSystem { text: String, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Error in converting a sticker to the desired type - #[snafu(display("Failed to convert sticker {}: {}", sticker, error))] - BadStickerConversion { - sticker: String, - error: String, - #[snafu(backtrace(true))] - back: Backtrace, + back: backtrace::Backtrace, }, } -error_from!(crate::commands::Error); -error_from!(regex::Error); -error_from!(std::io::Error); -error_from!(std::num::ParseFloatError); -error_from!(std::num::ParseIntError); -error_from!(std::string::FromUtf8Error); +impl std::convert::From for Error { + fn from(err: std::io::Error) -> Self { + Error::Io { + source: err, + back: Backtrace::new(), + } + } +} + +// Implementation helper: there is a lot of code in this module that reads: "(do thing that returns +// Option).ok_or_else(|| create an Error::Protocol instance).and then..." This private trait helps +// make that a bit more succinct. I largely lifted it from Snafu. +trait OptionExt: Sized { + fn context(self, op: Operation, msg: &str) -> std::result::Result; +} + +impl OptionExt for Option { + fn context(self, op: Operation, msg: &str) -> std::result::Result { + self.ok_or_else(|| Error::Protocol { + op: op, + msg: String::from(msg), // msg is likely borrowed, sadly + back: Backtrace::new(), + }) + } +} + +// This impl lets you change a bool into an Error::Protocol, as well: "(boolean +// assertion).context(...)" +impl OptionExt<()> for bool { + fn context(self, op: Operation, msg: &str) -> std::result::Result<(), Error> { + if self { + Ok(()) + } else { + Err(Error::Protocol { + op: op, + msg: String::from(msg), + back: Backtrace::new(), + }) + } + } +} + +// Same for Result-s! +trait ResultExt: Sized { + fn context(self, op: Operation, msg: &str) -> std::result::Result; +} + +impl ResultExt for std::result::Result { + fn context(self, op: Operation, msg: &str) -> std::result::Result { + self.map_err(|_| Error::Protocol { + op: op, + msg: String::from(msg), + back: Backtrace::new(), + }) + } +} + +impl std::fmt::Display for Error { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::Protocol { op, msg, back: _ } => write!(f, "Protocol error ({}): {}", op, msg), + Error::Io { source, back: _ } => write!(f, "I/O error: {}", source), + Error::Encoding { source, back: _ } => write!(f, "Encoding error: {}", source), + Error::StickerConversion { + sticker, + source, + back: _, + } => write!(f, "While converting sticker ``{}'': {}", sticker, source), + Error::IdleSubSystem { text, back: _ } => { + write!(f, "``{}'' is not a recognized Idle subsystem", text) + } + _ => write!(f, "Unknown client error"), + } + } +} + +// Stolen shamelessly from Snafu +// +// this trait turns the receiver into an Error trait object. +pub trait AsErrorSource { + /// For maximum effectiveness, this needs to be called as a method + /// to benefit from Rust's automatic dereferencing of method + /// receivers. + fn as_error_source(&self) -> &(dyn std::error::Error + 'static); +} + +impl AsErrorSource for dyn std::error::Error + Send + Sync + 'static { + fn as_error_source(&self) -> &(dyn std::error::Error + 'static) { + self + } +} + +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match &self { + Error::Io { + ref source, + back: _, + } => Some(source), + Error::Encoding { + ref source, + back: _, + } => Some(source), + Error::StickerConversion { + sticker: _, + ref source, + back: _, + } => Some(source.as_error_source()), + _ => None, + } + } +} pub type Result = std::result::Result; @@ -259,6 +278,8 @@ impl CurrentSong { } } +/// The MPD player itself can be in one of three states: playing, paused or stopped. In the first +/// two there is a "current" song. #[derive(Clone, Debug)] pub enum PlayerStatus { Play(CurrentSong), @@ -279,11 +300,13 @@ impl PlayerStatus { // Connection // //////////////////////////////////////////////////////////////////////////////////////////////////// -/// Represents a simple, textual request/response protocol like that employed by [`mpd`]. -/// -/// This trait also enables unit testing client implementations. Note that it is async. +/// A trait representing a simple, textual request/response protocol like that +/// [employed](https://www.musicpd.org/doc/html/protocol.html) by [MPD](https://www.musicpd.org/): +/// the caller sends a textual command & the server responds with a (perhaps multi-line) textual +/// response. /// -/// [`mpd`]: https://www.musicpd.org/doc/html/protocol.html +/// This trait also enables unit testing client implementations. Note that it is async-- cf. +/// [async_trait](https://docs.rs/async-trait/latest/async_trait/). #[async_trait] pub trait RequestResponse { async fn req(&mut self, msg: &str) -> Result; @@ -340,10 +363,45 @@ pub mod test_mock { } } -/// mpd connections talk one protocol over either a TCP or a Unix socket +/// [MPD](https://www.musicpd.org/) connections talk the same +/// [protocol](https://www.musicpd.org/doc/html/protocol.html) over either a TCP or a Unix socket. +/// +/// # Examples +/// +/// Implementations are provided for tokio [UnixStream] and [TcpStream], but [MpdConnection] is a +/// trait that can work in terms of any asynchronous communications channel (so long as it is also +/// [Send] and [Unpin] so async executors can pass them between threads. +/// +/// To create a connection to an `MPD` server over a Unix domain socket: +/// +/// ```no_run +/// use std::path::Path; +/// use tokio::net::UnixStream; +/// use mpdpopm::clients::MpdConnection; +/// let local_conn = MpdConnection::::new(Path::new("/var/run/mpd/mpd.sock")); +/// ``` +/// +/// In this example, `local_conn` is a Future that will resolve to a Result containing the +/// [MpdConnection] Unix domain socket implementation once the socket has been established, the MPD +/// server greets us & the protocol version has been parsed. +/// +/// or over a TCP socket: +/// +/// ```no_run +/// use std::net::SocketAddrV4; +/// use tokio::net::{TcpStream, ToSocketAddrs}; +/// use mpdpopm::clients::MpdConnection; +/// let tcp_conn = MpdConnection::::new("localhost:6600".parse::().unwrap()); +/// ``` +/// +/// Here, `tcp_conn` is a Future that will resolve to a Result containing the [MpdConnection] TCP +/// implementation on successful connection to the MPD server (i.e. the connection is established, +/// the server greets us & we parse the protocol version). +/// +/// pub struct MpdConnection { sock: T, - _proto_ver: String, + _protocol_ver: String, } /// MpdConnection implements RequestResponse using the usual (async) socket I/O @@ -395,7 +453,10 @@ where } } - Ok(String::from_utf8(buf)?) + Ok(String::from_utf8(buf).map_err(|err| Error::Encoding { + source: err, + back: Backtrace::new(), + })?) } } @@ -406,10 +467,12 @@ where { let mut buf = Vec::with_capacity(32); let _cb = sock.read_buf(&mut buf).await?; - let text = String::from_utf8(buf)?; - text.starts_with("OK MPD ").as_option().context(Connect { - text: text.trim().to_string(), + let text = String::from_utf8(buf).map_err(|err| Error::Encoding { + source: err, + back: Backtrace::new(), })?; + text.starts_with("OK MPD ") + .context(Operation::Connect, text.trim())?; info!("Connected {}.", text[7..].trim()); Ok(text[7..].trim().to_string()) } @@ -420,18 +483,19 @@ impl MpdConnection { let proto_ver = parse_connect_rsp(&mut sock).await?; Ok(Box::new(MpdConnection:: { sock: sock, - _proto_ver: proto_ver, + _protocol_ver: proto_ver, })) } } impl MpdConnection { + // NTS: we have to box the return value because a `dyn RequestResponse` isn't Sized. pub async fn new>(pth: P) -> Result> { let mut sock = UnixStream::connect(pth).await?; let proto_ver = parse_connect_rsp(&mut sock).await?; Ok(Box::new(MpdConnection:: { sock: sock, - _proto_ver: proto_ver, + _protocol_ver: proto_ver, })) } } @@ -457,29 +521,45 @@ pub fn quote(text: &str) -> String { // Client // //////////////////////////////////////////////////////////////////////////////////////////////////// -/// General-purpose [`mpd`] client. "General-purpose" in the sense that we send commands through -/// it; the interface is narrowly scoped to this program's needs. +/// General-purpose [mpd](https://www.musicpd.org) +/// [client](https://www.musicpd.org/doc/html/protocol.html): "general-purpose" in the sense that we +/// send commands through it; the interface is narrowly scoped to this program's needs. +/// +/// # Introduction +/// +/// This is the primary abstraction of the MPD client protocol, written for the convenience of +/// [mpdpopm](crate). Construct instances with a TCP socket, a Unix socket, or any [RequestResponse] +/// implementation. You can then carry out assorted operations in the MPD client protocol by +/// invoking its methods. +/// +/// ```no_run +/// use std::path::Path; +/// use mpdpopm::clients::Client; +/// let client = Client::open(Path::new("/var/run/mpd.sock")); +/// ``` /// -/// Construct instances with a TCP socket, a Unix socket, or any [`RequestResponse`] implementation. -/// I hate carrying the regexp's around with each instance; ideally they'd be constructed once, -/// preferrably at compilation time, but at least a program start-up, and re-used, but I can't -/// figure out how to do that in Rust. +/// `client` is now a [Future](https://doc.rust-lang.org/stable/std/future/trait.Future.html) that +/// resolves to a [Client] instance talking to `/var/run/mpd.sock`. /// -/// [`mpd`]: https://www.musicpd.org/doc/html/protocol.html +/// ```no_run +/// use mpdpopm::clients::Client; +/// let client = Client::connect("localhost:6600"); +/// ``` +/// +/// `client` is now a [Future](https://doc.rust-lang.org/stable/std/future/trait.Future.html) that +/// resolves to a [Client] instance talking TCP to the MPD server on localhost at port 6600. pub struct Client { stream: Box, - re_state: Regex, - re_songid: Regex, - re_elapsed: Regex, - re_file: Regex, - re_duration: Regex, } -const RE_STATE: &str = r"(?m)^state: (play|pause|stop)$"; -const RE_SONGID: &str = r"(?m)^songid: ([0-9]+)$"; -const RE_ELAPSED: &str = r"(?m)^elapsed: ([.0-9]+)$"; -const RE_FILE: &str = r"(?m)^file: (.*)$"; -const RE_DURATION: &str = r"(?m)^duration: (.*)$"; +// Thanks to +lazy_static! { + static ref RE_STATE: regex::Regex = Regex::new(r"(?m)^state: (play|pause|stop)$").unwrap(); + static ref RE_SONGID: regex::Regex = Regex::new(r"(?m)^songid: ([0-9]+)$").unwrap(); + static ref RE_ELAPSED: regex::Regex = Regex::new(r"(?m)^elapsed: ([.0-9]+)$").unwrap(); + static ref RE_FILE: regex::Regex = Regex::new(r"(?m)^file: (.*)$").unwrap(); + static ref RE_DURATION: regex::Regex = Regex::new(r"(?m)^duration: (.*)$").unwrap(); +} impl Client { pub async fn connect(addrs: A) -> Result { @@ -491,35 +571,10 @@ impl Client { } pub fn new(stream: Box) -> Result { - Ok(Client { - stream: stream, - re_state: Regex::new(&RE_STATE)?, - re_songid: Regex::new(RE_SONGID)?, - re_elapsed: Regex::new(RE_ELAPSED)?, - re_file: Regex::new(RE_FILE)?, - re_duration: Regex::new(RE_DURATION)?, - }) + Ok(Client { stream: stream }) } } -/// Utility function that applies the given regexp to `text`, and returns the first sub-group. -/// -/// The two context selectors shouldn't be needed, but they don't implement Clone, and I can't -/// get snafu's `context` to borrow them. -fn cap(re: ®ex::Regex, text: &str, ctx1: C1, ctx2: C2) -> Result -where - C1: snafu::IntoError, - C2: snafu::IntoError, -{ - Ok(re - .captures(text) - .context(ctx1)? - .get(1) - .context(ctx2)? - .as_str() - .to_string()) -} - impl Client { /// Retrieve the current server status. pub async fn status(&mut self) -> Result { @@ -530,71 +585,55 @@ impl Client { let text = self.stream.req("status").await?; // I first thought to avoid the use (and cost) of regular expressions by just doing - // sub-string searching on "state: ", but when I realized I needed to only match - // at the beginning of a line bailed & just went ahead. This makes for more succinct - // code, since I can't count on order, either. - let state = cap( - &self.re_state, - &text, - PlayState { - text: text.to_string(), - }, - PlayState { - text: text.to_string(), - }, - )?; - - match state.as_str() { + // sub-string searching on "state: ", but when I realized I needed to only match at the + // beginning of a line I bailed & just went ahead. This makes for more succinct code, since + // I can't count on order, either. + let state = RE_STATE + .captures(&text) + .context(Operation::Status, &text)? + .get(1) + .context(Operation::Status, &text)? + .as_str(); + + match state { "stop" => Ok(PlayerStatus::Stopped), "play" | "pause" => { - let songid = cap( - &self.re_songid, - &text, - SongId { - text: text.to_string(), - }, - SongId { - text: text.to_string(), - }, - )? - .parse::()?; - let elapsed = cap( - &self.re_elapsed, - &text, - Elapsed { - text: text.to_string(), - }, - Elapsed { - text: text.to_string(), - }, - )? - .parse::()?; + let songid = RE_SONGID + .captures(&text) + .context(Operation::Status, &text)? + .get(1) + .context(Operation::Status, &text)? + .as_str() + .parse::() + .context(Operation::Status, &text)?; + + let elapsed = RE_ELAPSED + .captures(&text) + .context(Operation::Status, &text)? + .get(1) + .context(Operation::Status, &text)? + .as_str() + .parse::() + .context(Operation::Status, &text)?; // navigate from `songid'-- don't send a "currentsong" message-- the current song // could have changed let text = self.stream.req(&format!("playlistid {}", songid)).await?; - let file = cap( - &self.re_file, - &text, - SongFile { - text: text.to_string(), - }, - SongFile { - text: text.to_string(), - }, - )?; - let duration = cap( - &self.re_duration, - &text, - Duration { - text: text.to_string(), - }, - Duration { - text: text.to_string(), - }, - )? - .parse::()?; + let file = RE_FILE + .captures(&text) + .context(Operation::Status, &text)? + .get(1) + .context(Operation::Status, &text)? + .as_str(); + let duration = RE_DURATION + .captures(&text) + .context(Operation::Status, &text)? + .get(1) + .context(Operation::Status, &text)? + .as_str() + .parse::() + .context(Operation::Status, &text)?; let curr = CurrentSong::new(songid, PathBuf::from(file), elapsed, duration); @@ -604,21 +643,22 @@ impl Client { Ok(PlayerStatus::Pause(curr)) } } - _ => Err(Error::UnknownState { - state: state.to_string(), - back: Backtrace::generate(), + _ => Err(Error::Protocol { + op: Operation::Status, + msg: state.to_string(), + back: Backtrace::new(), }), } } - /// Retrieve a song sticker by name, as a string + /// Retrieve a song sticker by name pub async fn get_sticker( &mut self, file: &str, sticker_name: &str, ) -> Result> where - ::Err: std::error::Error, + ::Err: std::error::Error + Sync + Send + 'static, { let msg = format!("sticker get song \"{}\" \"{}\"", file, sticker_name); let text = self.stream.req(&msg).await?; @@ -629,27 +669,22 @@ impl Client { let s = text[prefix.len()..] .split('\n') .next() - .context(StickerGet { text: text.clone() })?; - let r = T::from_str(s); - match r { - Ok(t) => Ok(Some(t)), - Err(e) => Err(Error::BadStickerConversion { + .context(Operation::GetSticker, &msg)?; + Ok(Some(T::from_str(s).map_err(|e| { + Error::StickerConversion { sticker: String::from(sticker_name), - error: format!("{}", e), - back: Backtrace::generate(), - }), - } - } else if text.starts_with("ACK ") && text.ends_with("no such sticker\n") { - Ok(None) + source: Box::::from(e), + back: Backtrace::new(), + } + })?)) } else { - Err(Error::StickerGet { - text: text.to_string(), - back: Backtrace::generate(), - }) + (text.starts_with("ACK ") && text.ends_with("no such sticker\n")) + .context(Operation::GetSticker, &msg)?; + Ok(None) } } - /// Set a song sticker by name, as text + /// Set a song sticker by name pub async fn set_sticker( &mut self, file: &str, @@ -663,10 +698,7 @@ impl Client { let text = self.stream.req(&msg).await?; debug!("Sent `{}'; got `{}'", &msg, &text); - text.starts_with("OK").as_option().context(StickerSet { - text: text.to_string(), - })?; - Ok(()) + text.starts_with("OK").context(Operation::SetSticker, &msg) } /// Send a file to a playlist @@ -675,10 +707,8 @@ impl Client { let text = self.stream.req(&msg).await?; debug!("Sent `{}'; got `{}'.", &msg, &text); - text.starts_with("OK").as_option().context(StickerSet { - text: text.to_string(), - })?; - Ok(()) + text.starts_with("OK") + .context(Operation::SendToPlaylist, &msg) } /// Send an arbitrary message @@ -687,10 +717,8 @@ impl Client { let text = self.stream.req(&msg).await?; debug!("Sent `{}'; got `{}'.", &msg, &text); - text.starts_with("OK").as_option().context(SendMessage { - text: text.to_string(), - })?; - Ok(()) + text.starts_with("OK") + .context(Operation::SendMessage, &text) } /// Update a URI @@ -707,17 +735,11 @@ impl Client { // on failure. let prefix = "updating_db: "; - if text.starts_with(prefix) { - // Seems prolix to me - Ok(text[prefix.len()..].split('\n').collect::>()[0] - .to_string() - .parse::()?) - } else { - Err(Error::UpdateDB { - text: text.to_string(), - back: Backtrace::generate(), - }) - } + text.starts_with(prefix).context(Operation::Update, &text)?; + text[prefix.len()..].split('\n').collect::>()[0] + .to_string() + .parse::() + .context(Operation::Update, &text) } /// Get the list of stored playlists @@ -737,9 +759,10 @@ impl Client { // // ACK... if text.starts_with("ACK") { - Err(Error::GetStoredPlaylists { - text: text.to_string(), - back: Backtrace::generate(), + Err(Error::Protocol { + op: Operation::GetStoredPlaylists, + msg: text, + back: Backtrace::new(), }) } else { Ok(text @@ -770,9 +793,10 @@ impl Client { // // ACK... if text.starts_with("ACK") { - Err(Error::Find { - text: text.to_string(), - back: Backtrace::generate(), + Err(Error::Protocol { + op: Operation::RspToUris, + msg: String::from(text), + back: Backtrace::new(), }) } else { Ok(text @@ -849,19 +873,21 @@ impl Client { // ACK ... if text.starts_with("ACK") { - Err(Error::GetStickers { - text: text.to_string(), - back: Backtrace::generate(), + Err(Error::Protocol { + op: Operation::GetStickers, + msg: text, + back: Backtrace::new(), }) } else { let mut m = HashMap::new(); let mut lines = text.lines(); loop { - let file = lines.next().context(GetStickers { text: text.clone() })?; + let file = lines.next().context(Operation::GetStickers, &text)?; if "OK" == file { break; } - let val = lines.next().context(GetStickers { text: text.clone() })?; + let val = lines.next().context(Operation::GetStickers, &text)?; + m.insert( String::from(&file[6..]), String::from(&val[10 + sticker.len()..]), @@ -891,9 +917,10 @@ impl Client { // // or "ACK..." if text.starts_with("ACK") { - Err(Error::GetAllSongs { - text: text.to_string(), - back: Backtrace::generate(), + Err(Error::Protocol { + op: Operation::GetAllSongs, + msg: text, + back: Backtrace::new(), }) } else { Ok(text @@ -914,9 +941,7 @@ impl Client { let text = self.stream.req(&msg).await?; debug!("Sent `{}'; got `{}'.", &msg, &text); - text.starts_with("OK").as_option().context(Add { - text: text.to_string(), - })?; + text.starts_with("OK").context(Operation::Add, &text)?; Ok(()) } } @@ -1161,6 +1186,7 @@ OK // IdleClient // //////////////////////////////////////////////////////////////////////////////////////////////////// +#[non_exhaustive] #[derive(Debug, PartialEq, Eq)] pub enum IdleSubSystem { Player, @@ -1176,9 +1202,9 @@ impl TryFrom<&str> for IdleSubSystem { } else if x == "message" { Ok(IdleSubSystem::Message) } else { - Err(Error::IdleSubsystem { + Err(Error::IdleSubSystem { text: String::from(text), - back: Backtrace::generate(), + back: Backtrace::new(), }) } } @@ -1193,16 +1219,39 @@ impl fmt::Display for IdleSubSystem { } } -/// mpdpopm client for "idle" connections. +/// [MPD](https://www.musicpd.org) client for "idle" connections. +/// +/// # Introduction +/// +/// This is an MPD client designed to "idle": it opens a long-lived connection to the MPD server and +/// waits for MPD to respond with a message indicating that there's been a change to a subsystem of +/// interest. At present, there are only two subsystems in which [mpdpopm](crate) is interested: the player +/// & messages (cf. [IdleSubSystem]). +/// +/// ```no_run +/// use std::path::Path; +/// use tokio::runtime::Runtime; +/// use mpdpopm::clients::IdleClient; +/// +/// let mut rt = Runtime::new().unwrap(); +/// rt.block_on( async { +/// let mut client = IdleClient::open(Path::new("/var/run/mpd.sock")).await.unwrap(); +/// client.subscribe("player").await.unwrap(); +/// client.idle().await.unwrap(); +/// // Arrives here when the player's state changes +/// }) +/// ``` +/// +/// `client` is now a [Future](https://doc.rust-lang.org/stable/std/future/trait.Future.html) that +/// resolves to an [IdleClient] instance talking to `/var/run/mpd.sock`. /// -/// I should probably make this generic over all types implementing AsyncRead & AsyncWrite. pub struct IdleClient { conn: Box, } impl IdleClient { - /// Create a new [`mpdpopm::client::IdleClient`][`IdleClient`] instance from something that - /// implements [`ToSocketAddrs`][`tokio::net::ToSocketAddrs`] + /// Create a new [mpdpopm::client::IdleClient][IdleClient] instance from something that + /// implements [ToSocketAddrs][tokio::net::ToSocketAddrs] pub async fn connect(addr: A) -> Result { Self::new(MpdConnection::::new(addr).await?) } @@ -1219,9 +1268,7 @@ impl IdleClient { pub async fn subscribe(&mut self, chan: &str) -> Result<()> { let text = self.conn.req(&format!("subscribe {}", chan)).await?; debug!("Sent subscribe message for {}; got `{}'.", chan, text); - text.starts_with("OK").as_option().context(Subscribe { - text: String::from(text), - })?; + text.starts_with("OK").context(Operation::Connect, &text)?; debug!("Subscribed to {}.", chan); Ok(()) } @@ -1243,17 +1290,13 @@ impl IdleClient { // // We remain subscribed, but we need to send a new idle message. - text.starts_with("changed: ").as_option().context(Idle { - text: String::from(&text), - })?; - let idx = text.find('\n').context(Idle { - text: String::from(&text), - })?; + text.starts_with("changed: ") + .context(Operation::Idle, &text)?; + let idx = text.find('\n').context(Operation::Idle, &text)?; + let result = IdleSubSystem::try_from(&text[9..idx])?; let text = text[idx + 1..].to_string(); - text.starts_with("OK").as_option().context(Idle { - text: String::from(text), - })?; + text.starts_with("OK").context(Operation::Idle, &text)?; Ok(result) } @@ -1287,10 +1330,7 @@ impl IdleClient { match state { State::Init => { line.starts_with("channel: ") - .as_option() - .context(GetMessages { - text: String::from(line), - })?; + .context(Operation::GetMessages, &line)?; chan = String::from(&line[9..]); state = State::Running; } @@ -1315,17 +1355,19 @@ impl IdleClient { } state = State::Finished; } else { - return Err(Error::GetMessages { - text: String::from(line), - back: Backtrace::generate(), + return Err(Error::Protocol { + op: Operation::GetMessages, + msg: text, + back: Backtrace::new(), }); } } State::Finished => { // Should never be here! - return Err(Error::GetMessages { - text: String::from(line), - back: Backtrace::generate(), + return Err(Error::Protocol { + op: Operation::GetMessages, + msg: String::from(line), + back: Backtrace::new(), }); } } diff --git a/mpdpopm/src/commands.rs b/mpdpopm/src/commands.rs index b146fff..67b15cc 100644 --- a/mpdpopm/src/commands.rs +++ b/mpdpopm/src/commands.rs @@ -13,23 +13,25 @@ // You should have received a copy of the GNU General Public License along with mpdpopm. If not, // see . -//! commands -- running commands on the server +//! Running commands on the server //! //! # Introduction //! //! [mpdpopm](crate) allows for running arbitrary programs on the server in response to server //! events or external commands (to keep ID3 tags up-to-date when a song is rated, for -//! instance). This may seem like a vulnerability if your [mpd] server is listening on a socket, but +//! instance). This may seem like a vulnerability if your [MPD] server is listening on a socket, but //! it's not like callers can execute arbitrary code: certain events can trigger commands that you, -//! the [mpd] server owner have configured. +//! the [MPD] server owner have configured. //! -//! [mpd]: https://www.musicpd.org/ "MPD" +//! [MPD]: https://www.musicpd.org/ "MPD" //! //! # The Generalized Command Framework //! -//! In addition to the fixed commands, [mpdpopm](https://github.com/sp1ff/mpdpopm) provides a -//! generalized command framework by which the admin can map arbitrary commands to code execution on -//! the server-side. A generalized command can be described as: +//! In addition to fixed commands (i.e. commands that are run in response to particular events, like +//! rating a song, or completing a track), [mpdpopm](https://github.com/sp1ff/mpdpopm) provides a +//! generalized command framework by which you can map arbitrary messages sent to the +//! [mpdpopm](crate) channel to code execution on the server side. A generalized command can be +//! described as: //! //! 1. the command name: commands shall be named according to the regular expression: //! [a-zA-Z][-a-zA-Z0-9]+ @@ -37,9 +39,9 @@ //! 2. command parameters: a command may take parameters; the parameters are defined by an array //! of zero or more instances of: //! -//! - parameter name: parameter names shall match [a-zA-Z][-a-zA-Z0-9]+ +//! - parameter name: parameter names shall match the regex: [a-zA-Z][-a-zA-Z0-9]+ //! -//! - parameter type: parameters are typed: +//! - parameter type: parameters may be a: //! //! - general/string //! @@ -55,24 +57,26 @@ //! //! - replacement parameters //! -//! - %1, %2, %3... will be replaced with the parameter in the corresponding position above +//! - %1, %2, %3... will be replaced with the parameter in the corresponding position above //! -//! - %full-file will expand to the absolute path to the song named by the track argument, -//! if present; if this parameter is used, and the track is *not* provided in the -//! command arguments, it is an error +//! - %full-file will expand to the absolute path to the song named by the track argument, if +//! present; if this parameter is used, and the track is *not* provided in the command +//! arguments, it is an error //! -//! - %rating, %play-count & %last-played will expand to the value of the corresponding -//! sticker, if present. If any of these are used, and the track is *not* provided in -//! the command arguments, it is an error. If the corresponding sticker is not present in -//! the sticker database, default values of 0, 0, and Jan. 1 1970 (i.e. Unix epoch) will -//! be provided instead. +//! - %rating, %play-count & %last-played will expand to the value of the corresponding +//! sticker, if present. If any of these are used, and the track is *not* provided in the +//! command arguments, it is an error. If the corresponding sticker is not present in the +//! sticker database, default values of 0, 0, and Jan. 1 1970 (i.e. Unix epoch) will be +//! provided instead. //! //! ## Quoting //! -//! [mpd](https://www.musicpd.org/) breaks up commands into their constituent tokens on the basis +//! [MPD](https://www.musicpd.org/) breaks up commands into their constituent tokens on the basis //! of whitespace: //! +//! ```text //! token := [a-zA-Z0-09~!@#$%^&*()-=_+[]{}\|;:<>,./?]+ | "([ \t'a-zA-Z0-09~!@#$%^&*()-=_+[]{}|;:<>,./?]|\"|\\)" +//! ``` //! //! In other words, a token is any group of ASCII, non-whitespace characters except for ' & " (even //! \). In order to include space, tab, ' or " the token must be enclosed in double quotes at which @@ -85,19 +89,25 @@ //! illustrate. Suppose the command is named myfind and we wish to pass an argument which contains //! both ' characters & spaces: //! -//! myfind (Artist == 'foo' and rating > '***') -//! ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -//! cmd argument +//! ```text +//! myfind (Artist == 'foo' and rating > '***') +//! ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +//! cmd argument +//! ``` //! //! Since the argument contains spaces _and_ '-s we'll need to enclose it in double quotes (at //! which point the ' characters become permissible): //! -//! myfind "(Artist == 'foo' and rating > '***')" +//! ```text +//! myfind "(Artist == 'foo' and rating > '***')" +//! ``` //! //! Since this entire command will itself be a single token in the "sendmessage" command, we //! need to quote it in its entirety on the client side: //! -//! "myfind \"(Artist == 'foo' and rating > '***')\"" +//! ```text +//! "myfind \"(Artist == 'foo' and rating > '***')\"" +//! ``` //! //! (Enclose the entire command in double-quotes, then backslash-escape any " or \ characters //! therein). @@ -109,10 +119,10 @@ use crate::clients::PlayerStatus; +use backtrace::Backtrace; use futures::future::Future; use log::{debug, info}; use serde::{Deserialize, Serialize}; -use snafu::{OptionExt, Snafu}; use tokio::process::Command; use std::collections::HashMap; @@ -123,28 +133,62 @@ use std::path::PathBuf; // module Error type // //////////////////////////////////////////////////////////////////////////////////////////////////// -#[derive(Debug, Snafu)] +/// An mpdpopm command error +#[derive(Debug)] +#[non_exhaustive] pub enum Error { - #[snafu(display("The path `{}' cannot be converted to a UTF-8 string", pth.display()))] - BadPath { pth: PathBuf }, - #[snafu(display("Parameter {} is a duplciate track argument", index))] - DuplicateTrackArgument { index: usize }, - #[snafu(display("Missing parameter {} & it it is not marked as default", index))] - MissingParameter { index: usize }, - #[snafu(display("The current track was requested, but there is not current track"))] + BadPath { + pth: PathBuf, + back: backtrace::Backtrace, + }, + DuplicateTrackArgument { + index: usize, + back: backtrace::Backtrace, + }, + MissingParameter { + index: usize, + back: backtrace::Backtrace, + }, NoCurrentTrack, - #[snafu(display( - "This command is marked as needing to update the affected track only, but \" -there is no track given" - ))] NoTrackToUpdate, - #[snafu(display( - "The template string `{}' has a trailing '%' character, which is illegal.", - template - ))] - TrailingPercent { template: String }, - #[snafu(display("Unknown replacement parameter `{}'", param))] - UnknownParameter { param: String }, + TrailingPercent { + template: String, + back: backtrace::Backtrace, + }, + UnknownParameter { + param: String, + back: backtrace::Backtrace, + }, +} + +impl std::fmt::Display for Error { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::BadPath { pth, back: _ } => write!(f, "Bad path: {:?}", pth), + Error::DuplicateTrackArgument { index, back: _ } => { + write!(f, "Duplicate track argument at index {}", index) + } + Error::MissingParameter { index, back: _ } => { + write!(f, "Missing actual parameter at index {}", index) + } + Error::NoCurrentTrack => write!(f, "No current track"), + Error::NoTrackToUpdate => write!(f, "No track to update"), + Error::TrailingPercent { template, back: _ } => { + write!(f, "Trailing percent in template ``{}''", template) + } + Error::UnknownParameter { param, back: _ } => { + write!(f, "Unknown parameter ``{}''", param) + } + _ => write!(f, "Unknown commands error"), + } + } +} + +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Option::None + } } type Result = std::result::Result; @@ -168,8 +212,9 @@ pub fn process_replacements(templ: &str, params: &HashMap) -> Re if a != '%' { out.push(a); } else { - let b = c.peek().context(TrailingPercent { + let b = c.peek().ok_or_else(|| Error::TrailingPercent { template: String::from(templ), + back: Backtrace::new(), })?; if *b == '%' { c.next(); @@ -187,8 +232,9 @@ pub fn process_replacements(templ: &str, params: &HashMap) -> Re } }) .collect(); - out.push_str(params.get(&t).context(UnknownParameter { + out.push_str(params.get(&t).ok_or_else(|| Error::UnknownParameter { param: String::from(t), + back: Backtrace::new(), })?); match terminal { Some(x) => out.push(x), @@ -409,7 +455,10 @@ impl GeneralizedCommand { cfp.push(&curr.file); let cfs = cfp .to_str() - .context(BadPath { pth: cfp.clone() })? + .ok_or_else(|| Error::BadPath { + pth: cfp.clone(), + back: Backtrace::new(), + })? .to_string(); params.insert("current-file".to_string(), cfs.clone()); debug!("current-file is: {}", cfs); @@ -432,18 +481,27 @@ impl GeneralizedCommand { // Slightly more complicated, replacement parameter %i will be "", but only // if this formal parameter is allowed to be defaulted. if i < self.default_after { - return Err(Error::MissingParameter { index: i }); + return Err(Error::MissingParameter { + index: i, + back: Backtrace::new(), + }); } debug!("%{} is: nil", i); params.insert(format!("{}", i), String::from("")); } (FormalParameter::Track, Some(token)) => { if saw_track { - return Err(Error::DuplicateTrackArgument { index: i }); + return Err(Error::DuplicateTrackArgument { + index: i, + back: Backtrace::new(), + }); } let mut ffp = self.music_dir.clone(); ffp.push(PathBuf::from(token)); - let ffs = ffp.to_str().context(BadPath { pth: ffp.clone() })?; + let ffs = ffp.to_str().ok_or_else(|| Error::BadPath { + pth: ffp.clone(), + back: Backtrace::new(), + })?; params.insert(format!("{}", i), ffs.to_string()); params.insert("full-file".to_string(), ffs.to_string()); full_file = Some(ffs.to_string()); @@ -451,10 +509,16 @@ impl GeneralizedCommand { } (FormalParameter::Track, None) => { if i < self.default_after { - return Err(Error::MissingParameter { index: i }); + return Err(Error::MissingParameter { + index: i, + back: Backtrace::new(), + }); } if saw_track { - return Err(Error::DuplicateTrackArgument { index: i }); + return Err(Error::DuplicateTrackArgument { + index: i, + back: Backtrace::new(), + }); } match ¤t_file { Some(cf) => { diff --git a/mpdpopm/src/config.rs b/mpdpopm/src/config.rs index 482bc9a..2107b9e 100644 --- a/mpdpopm/src/config.rs +++ b/mpdpopm/src/config.rs @@ -21,7 +21,7 @@ //! //! ## Discussion //! -//! In the first releases of [mpdpopm] I foolishly forgot to add a version field to the +//! In the first releases of [mpdpopm](crate) I foolishly forgot to add a version field to the //! configuration structure. I am now paying for my sin by having to attempt serializing two //! versions until one succeeds. //! @@ -42,7 +42,6 @@ use crate::commands::{FormalParameter, Update}; use crate::vars::{LOCALSTATEDIR, PREFIX}; use serde::{Deserialize, Serialize}; -use snafu::Snafu; use std::path::PathBuf; @@ -90,7 +89,7 @@ pub struct Config0 { /// Args, with replacement parameters, for the playcount command playcount_command_args: Vec, /// Sticker under which to store song ratings, as a textual representation of a number in - /// [0,255] + /// `[0,255]` rating_sticker: String, /// Command, with replacement parameters, to be run when a song is rated ratings_command: String, @@ -100,7 +99,7 @@ pub struct Config0 { gen_cmds: Vec, } -/// [mpdpopm] can communicate with MPD over either a local Unix socket, or over regular TCP +/// [mpdpopm](crate) can communicate with MPD over either a local Unix socket, or over regular TCP #[derive(Debug, Deserialize, PartialEq, Serialize)] pub enum Connection { /// Local Unix socket-- payload is the path to the socket @@ -160,7 +159,7 @@ pub struct Config { _version: String, /// Location of log file pub log: PathBuf, - /// How to connceto to mpd + /// How to connect to mpd pub conn: Connection, /// The `mpd' root music directory, relative to the host on which *this* daemon is running pub local_music_dir: PathBuf, @@ -180,7 +179,7 @@ pub struct Config { /// Args, with replacement parameters, for the playcount command pub playcount_command_args: Vec, /// Sticker under which to store song ratings, as a textual representation of a number in - /// [0,255] + /// `[0,255]` pub rating_sticker: String, /// Command, with replacement parameters, to be run when a song is rated pub ratings_command: String, @@ -237,24 +236,25 @@ impl From for Config { } } -// https://github.com/rotty/lexpr-rs/issues/77 -// #[derive(Debug, Deserialize, Serialize)] -// #[serde(tag = "version")] -// enum Configurations { -// #[serde(rename = "1")] -// V1(Config), -// } - -#[derive(Debug, Snafu)] +#[derive(Debug)] pub enum Error { /// Failure to parse - #[snafu(display("Failed to parse configuration: `{}', `{}'", outer, inner))] ParseFail { outer: Box, inner: Box, }, } +impl std::fmt::Display for Error { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::ParseFail { outer, inner: _ } => write!(f, "Parse failure: {}", outer), + _ => write!(f, "Unknown configuration error"), + } + } +} + pub type Result = std::result::Result; pub fn from_str(text: &str) -> Result { diff --git a/mpdpopm/src/error_from.rs b/mpdpopm/src/error_from.rs deleted file mode 100644 index 9756dcc..0000000 --- a/mpdpopm/src/error_from.rs +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright (C) 2020-2021 Michael Herstine -// -// This file is part of mpdpopm. -// -// mpdpopm is free software: you can redistribute it and/or modify it under the terms of the GNU -// General Public License as published by the Free Software Foundation, either version 3 of the -// License, or (at your option) any later version. -// -// mpdpopm 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 General -// Public License for more details. -// -// You should have received a copy of the GNU General Public License along with mpdpopm. If not, -// see . - -/// Provide an implementation of std::convert::From for it's argument for type Error -#[macro_export] -macro_rules! error_from { - ($t:ty) => { - impl std::convert::From<$t> for Error { - fn from(err: $t) -> Self { - Error::Other { - cause: Box::new(err), - back: Backtrace::generate(), - } - } - } - }; -} diff --git a/mpdpopm/src/filters_ast.rs b/mpdpopm/src/filters_ast.rs index afbc589..2b433fc 100644 --- a/mpdpopm/src/filters_ast.rs +++ b/mpdpopm/src/filters_ast.rs @@ -13,17 +13,16 @@ // You should have received a copy of the GNU General Public License along with mpdpopm. If not, // see . -//! filters-ast -- Types for building the Abstract Syntax Tree when parsing filters +//! Types for building the Abstract Syntax Tree when parsing filters +//! +//! This module provides support for our [lalrpop](https://github.com/lalrpop/lalrpop) grammar. use crate::clients::Client; -use crate::error_from; +use backtrace::Backtrace; use boolinator::Boolinator; use chrono::prelude::*; use log::debug; -use snafu::{Backtrace, GenerateBacktrace, Snafu}; - -extern crate regex; use std::collections::{HashMap, HashSet}; use std::str::FromStr; @@ -318,62 +317,102 @@ pub enum EvalOp { Not, } -#[derive(Debug, Snafu)] +impl std::fmt::Display for EvalOp { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + EvalOp::And => write!(f, "And"), + EvalOp::Or => write!(f, "Or"), + EvalOp::Not => write!(f, "Not"), + } + } +} + +#[derive(Debug)] pub enum Error { - #[snafu(display("{}", cause))] - Other { - #[snafu(source(true))] - cause: Box, - #[snafu(backtrace(true))] - back: Backtrace, - }, - #[snafu(display("'{}' could not be parsed as an ISO 8601 timestamp.", text))] BadISO8601String { - text: String, - #[snafu(backtrace(true))] + text: Vec, back: Backtrace, }, - #[snafu(display("'{}' could not be parsed as quoted text.", text))] ExpectQuoted { text: String, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("While evaluating a filter, {}.", text))] FilterTypeErr { text: String, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("Operator {:#?} is not valid in this context.", op))] InvalidOperand { op: OpCode, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("Operator {:#?} left on the stack.", op))] OperatorOnStack { op: EvalOp, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("Rating {} out-of-range (0-255, inclusive).", rating))] RatingOverflow { rating: usize, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("{} operands left on the stack; check the logs for details.", num_ops))] TooManyOperands { num_ops: usize, - #[snafu(backtrace(true))] + back: Backtrace, + }, + NumericParse { + sticker: String, + source: std::num::ParseIntError, + back: Backtrace, + }, + Client { + source: crate::clients::Error, back: Backtrace, }, } -error_from!(crate::clients::Error); -error_from!(std::num::ParseIntError); -error_from!(std::str::Utf8Error); +impl std::fmt::Display for Error { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::BadISO8601String { text, back: _ } => { + write!(f, "Bad ISO8601 timestamp: ``{:?}''", text) + } + Error::ExpectQuoted { text, back: _ } => write!(f, "Expected quote: ``{}''", text), + Error::FilterTypeErr { text, back: _ } => { + write!(f, "Un-expected type in filter ``{}''", text) + } + Error::InvalidOperand { op, back: _ } => write!(f, "Invalid operand {}", op), + Error::OperatorOnStack { op, back: _ } => { + write!(f, "Operator {} left on parse stack", op) + } + Error::RatingOverflow { rating, back: _ } => write!(f, "Rating {} overflows", rating), + Error::TooManyOperands { num_ops, back: _ } => { + write!(f, "Too many operands ({})", num_ops) + } + Error::NumericParse { + sticker, + source, + back: _, + } => write!(f, "While parsing sticker {}, got {}", sticker, source), + Error::Client { source, back: _ } => write!(f, "Client error: {}", source), + } + } +} + +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match &self { + Error::NumericParse { + sticker: _, + ref source, + back: _, + } => Some(source), + Error::Client { + ref source, + back: _, + } => Some(source), + _ => None, + } + } +} pub type Result = std::result::Result; @@ -384,15 +423,44 @@ fn peek(buf: &[u8]) -> Option { } } -// PRE: `buf' is ASCII -fn take<'a>(buf: &'a mut &[u8], i: usize) -> Option<&'a str> { - if i <= buf.len() { - let (first, second) = buf.split_at(i); - *buf = second; - Some(std::str::from_utf8(first).unwrap()) - } else { - None +// advancing a slice by `i` indicies can *not* be this difficult +/// Pop a single byte off of `buf` +fn take1(buf: &mut &[u8], i: usize) -> Result<()> { + if i > buf.len() { + return Err(Error::BadISO8601String { + text: buf.to_vec(), + back: Backtrace::new(), + }); + } + let (_first, second) = buf.split_at(i); + *buf = second; + Ok(()) +} + +/// Pop `i` bytes off of `buf` & parse them as a T +fn take2(buf: &mut &[u8], i: usize) -> Result +where + T: FromStr, +{ + // 1. check len + if i > buf.len() { + return Err(Error::BadISO8601String { + text: buf.to_vec(), + back: Backtrace::new(), + }); } + let (first, second) = buf.split_at(i); + *buf = second; + // 2. convert to a string + let s = std::str::from_utf8(first).map_err(|_| Error::BadISO8601String { + text: buf.to_vec(), + back: Backtrace::new(), + })?; + // 3. parse as a T + s.parse::().map_err(|_err| Error::BadISO8601String { + text: buf.to_vec(), + back: Backtrace::new(), + }) // Parse*Error => Error } /// Parse a timestamp in ISO 8601 format to a chrono DateTime instance @@ -402,17 +470,12 @@ fn take<'a>(buf: &'a mut &[u8], i: usize) -> Option<&'a str> { /// parse output in any event. The ISO 8601 format is simple enough that I've chosen to simply /// hand-parse it. pub fn parse_iso_8601(mut buf: &mut &[u8]) -> Result { + // I wonder if `nom` would be a better choice? + // The first four characters must be the year (expanded year representation is not supported by // this parser). - let year = match take(&mut buf, 4) { - Some(s) => s.parse::()?, - None => { - return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), - }); - } - }; + + let year: i32 = take2(&mut buf, 4)?; // Now at this point: // 1. we may be done (i.e. buf.len() == 0) @@ -432,20 +495,10 @@ pub fn parse_iso_8601(mut buf: &mut &[u8]) -> Result { if next != Some('T') { let mut ext_fmt = false; if next == Some('-') { - take(&mut buf, 1); + take1(buf, 1)?; ext_fmt = true; } - match take(&mut buf, 2) { - Some(s) => { - month = s.parse::()?; - } - None => { - return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), - }); - } - } + month = take2(&mut buf, 2)?; // At this point: // 1. we may be done (i.e. buf.len() == 0) @@ -454,70 +507,30 @@ pub fn parse_iso_8601(mut buf: &mut &[u8]) -> Result { if buf.len() != 0 { if peek(buf) != Some('T') { if ext_fmt { - take(&mut buf, 1); - } - match take(&mut buf, 2) { - Some(s) => { - day = s.parse::()?; - } - None => { - return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), - }); - } + take1(&mut buf, 1)?; } + day = take2(&mut buf, 2)?; } } } // Parse time: at this point, buf will either be empty or begin with 'T' if buf.len() != 0 { - take(&mut buf, 1); + take1(&mut buf, 1)?; // If there's a T, there must at least be an hour - match take(&mut buf, 2) { - Some(s) => { - hour = s.parse::()?; - } - None => { - return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), - }); - } - } + hour = take2(&mut buf, 2)?; if buf.len() != 0 { let mut ext_fmt = false; if peek(buf) == Some(':') { - take(&mut buf, 1); + take1(&mut buf, 1)?; ext_fmt = true; } - match take(&mut buf, 2) { - Some(s) => { - minute = s.parse::()?; - } - None => { - return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), - }); - } - } + minute = take2(&mut buf, 2)?; if buf.len() != 0 { if ext_fmt { - take(&mut buf, 1); - } - match take(&mut buf, 2) { - Some(s) => { - second = s.parse::()?; - } - None => { - return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), - }); - } + take1(&mut buf, 1)?; } + second = take2(&mut buf, 2)?; } } } @@ -533,39 +546,21 @@ pub fn parse_iso_8601(mut buf: &mut &[u8]) -> Result { let next = peek(buf); if next != Some('-') && next != Some('+') { return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), + text: buf.to_vec(), + back: Backtrace::new(), }); } let west = next == Some('-'); - take(&mut buf, 1); - - let hours = match take(&mut buf, 2) { - Some(s) => s.parse::()?, - None => { - return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), - }); - } - }; + take1(&mut buf, 1)?; + + let hours: i32 = take2(&mut buf, 2)?; let mut minutes = 0; if buf.len() > 0 { if peek(buf) == Some(':') { - take(&mut buf, 1); - } - match take(&mut buf, 2) { - Some(s) => { - minutes = s.parse::()?; - } - None => { - return Err(Error::BadISO8601String { - text: String::from(std::str::from_utf8(buf)?), - back: Backtrace::generate(), - }); - } + take1(&mut buf, 1)?; } + minutes = take2(&mut buf, 2)?; } if west { @@ -637,7 +632,7 @@ pub fn expect_quoted(qtext: &str) -> Result { if quote != Some('\'') && quote != Some('"') { return Err(Error::ExpectQuoted { text: String::from(qtext), - back: Backtrace::generate(), + back: Backtrace::new(), }); } @@ -655,7 +650,7 @@ pub fn expect_quoted(qtext: &str) -> Result { None => { return Err(Error::ExpectQuoted { text: String::from(qtext), - back: Backtrace::generate(), + back: Backtrace::new(), }); } } @@ -706,7 +701,7 @@ fn make_numeric_closure<'a, T: 'a + PartialEq + PartialOrd + Copy>( OpCode::LessThanEqual => Ok(Box::new(move |x: T| x <= val) as Box bool>), _ => Err(Error::InvalidOperand { op: op, - back: Backtrace::generate(), + back: Backtrace::new(), }), } } @@ -714,7 +709,7 @@ fn make_numeric_closure<'a, T: 'a + PartialEq + PartialOrd + Copy>( async fn eval_numeric_sticker_term< // The `FromStr' trait bound is really weird, but if I don't constrain the associated // Err type to be `ParseIntError' the compiler complains about not being able to convert - // it to type `Error'. I'm probably still "thinking in C++" and imaginging the compiler + // it to type `Error'. I'm probably still "thinking in C++" and imagining the compiler // instantiating this function for each type (u8, usize, &c) instead of realizing that the Rust // compiler is processing this as a first-class function. // @@ -743,20 +738,37 @@ async fn eval_numeric_sticker_term< // of a collection. let mut m = client .get_stickers(sticker) - .await? + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? .drain() .map(|(k, v)| v.parse::().map(|x| (k, x))) - .collect::, _>>()?; + .collect::, _>>() + .map_err(|err| Error::NumericParse { + sticker: String::from(sticker), + source: err, + back: Backtrace::new(), + })?; // `m' is now a map of song URI to rating/playcount/wathever (expressed as a T)... for all songs // that have the salient sticker. // // This seems horribly inefficient, but I'm going to fetch all the song URIs in the music DB, // and augment `m' with entries of `default_val' for any that are not already there. - client.get_all_songs().await?.drain(..).for_each(|song| { - if m.get(&song).is_none() { - m.insert(song, default_val); - } - }); + client + .get_all_songs() + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? + .drain(..) + .for_each(|song| { + if m.get(&song).is_none() { + m.insert(song, default_val); + } + }); // Now that we don't have to worry about operations that can fail, we can use // `filter_map'. Ok(m.drain() @@ -801,7 +813,11 @@ async fn eval_term<'a>( match term { Term::UnaryCondition(op, val) => Ok(client .find1(&format!("{}", op), "e_value(&val), case) - .await? + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? .drain(..) .collect()), Term::BinaryCondition(attr, op, val) => { @@ -811,7 +827,7 @@ async fn eval_term<'a>( if *n > 255 { return Err(Error::RatingOverflow { rating: *n, - back: Backtrace::generate(), + back: Backtrace::new(), }); } Ok(eval_numeric_sticker_term( @@ -825,7 +841,7 @@ async fn eval_term<'a>( } _ => Err(Error::FilterTypeErr { text: format!("filter ratings expect an unsigned int; got {:#?}", val), - back: Backtrace::generate(), + back: Backtrace::new(), }), } } else if *attr == Selector::PlayCount { @@ -840,7 +856,7 @@ async fn eval_term<'a>( .await?), _ => Err(Error::FilterTypeErr { text: format!("filter ratings expect an unsigned int; got {:#?}", val), - back: Backtrace::generate(), + back: Backtrace::new(), }), } } else if *attr == Selector::LastPlayed { @@ -855,7 +871,7 @@ async fn eval_term<'a>( .await?), _ => Err(Error::FilterTypeErr { text: format!("filter ratings expect an unsigned int; got {:#?}", val), - back: Backtrace::generate(), + back: Backtrace::new(), }), } } else { @@ -866,7 +882,11 @@ async fn eval_term<'a>( "e_value(val), case, ) - .await? + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? .drain(..) .collect()) } @@ -887,7 +907,11 @@ async fn negate_result( ) -> std::result::Result, Error> { Ok(client .get_all_songs() - .await? + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? .drain(..) .filter_map(|song| { // Some(thing) adds thing, None elides it @@ -1073,7 +1097,7 @@ pub async fn evaluate<'a>( .for_each(|(i, x)| debug!(" {}: {:#?}", i, x)); return Err(Error::TooManyOperands { num_ops: se.len(), - back: Backtrace::generate(), + back: Backtrace::new(), }); } @@ -1084,7 +1108,7 @@ pub async fn evaluate<'a>( debug!("Operator left on stack (!?): {:#?}", op); Err(Error::OperatorOnStack { op: op, - back: Backtrace::generate(), + back: Backtrace::new(), }) } } diff --git a/mpdpopm/src/lib.rs b/mpdpopm/src/lib.rs index db7244b..3952f5c 100644 --- a/mpdpopm/src/lib.rs +++ b/mpdpopm/src/lib.rs @@ -35,7 +35,6 @@ pub mod clients; pub mod commands; pub mod config; -pub mod error_from; pub mod filters_ast; pub mod messages; pub mod playcounts; @@ -54,6 +53,7 @@ use filters_ast::FilterStickerNames; use messages::MessageProcessor; use playcounts::PlayState; +use backtrace::Backtrace; use futures::{ future::FutureExt, pin_mut, select, @@ -61,7 +61,6 @@ use futures::{ }; use libc::getpid; use log::{debug, error, info}; -use snafu::{Backtrace, GenerateBacktrace, OptionExt, Snafu}; use tokio::{ signal, signal::unix::{signal, SignalKind}, @@ -72,25 +71,44 @@ use std::path::PathBuf; //////////////////////////////////////////////////////////////////////////////////////////////////// -#[derive(Debug, Snafu)] +#[derive(Debug)] +#[non_exhaustive] pub enum Error { - #[snafu(display("The path `{}' cannot be converted to a UTF-8 string", pth.display()))] - BadPath { pth: PathBuf }, - #[snafu(display("{}", cause))] - Other { - #[snafu(source(true))] - cause: Box, - #[snafu(backtrace(true))] + BadPath { + pth: PathBuf, + }, + Client { + source: crate::clients::Error, + back: Backtrace, + }, + Playcounts { + source: crate::playcounts::Error, back: Backtrace, }, } -error_from!(commands::Error); -error_from!(clients::Error); -error_from!(playcounts::Error); -error_from!(ratings::Error); -error_from!(std::num::ParseIntError); -error_from!(std::time::SystemTimeError); +impl std::fmt::Display for Error { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::BadPath { pth } => write!(f, "Bad path: {:?}", pth), + Error::Client { source, back: _ } => write!(f, "Client error: {}", source), + Error::Playcounts { source, back: _ } => write!(f, "Playcount error: {}", source), + } + } +} + +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match &self { + Error::Client { + ref source, + back: _, + } => Some(source), + _ => None, + } + } +} pub type Result = std::result::Result; @@ -102,7 +120,7 @@ pub async fn mpdpopm(cfg: Config) -> std::result::Result<(), Error> { info!("mpdpopm {} beginning (PID {}).", vars::VERSION, pid); // We need the music directory to be convertible to string; check that first-off: - let music_dir = cfg.local_music_dir.to_str().context(BadPath { + let music_dir = cfg.local_music_dir.to_str().ok_or_else(|| Error::BadPath { pth: cfg.local_music_dir.clone(), })?; @@ -113,8 +131,18 @@ pub async fn mpdpopm(cfg: Config) -> std::result::Result<(), Error> { ); let mut client = match cfg.conn { - Connection::Local { ref path } => Client::open(path).await?, - Connection::TCP { ref host, port } => Client::connect(format!("{}:{}", host, port)).await?, + Connection::Local { ref path } => { + Client::open(path).await.map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? + } + Connection::TCP { ref host, port } => Client::connect(format!("{}:{}", host, port)) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?, }; let mut state = PlayState::new( @@ -123,16 +151,34 @@ pub async fn mpdpopm(cfg: Config) -> std::result::Result<(), Error> { &cfg.lastplayed_sticker, cfg.played_thresh, ) - .await?; + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; let mut idle_client = match cfg.conn { - Connection::Local { ref path } => IdleClient::open(path).await?, - Connection::TCP { ref host, port } => { - IdleClient::connect(format!("{}:{}", host, port)).await? + Connection::Local { ref path } => { + IdleClient::open(path).await.map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? } + Connection::TCP { ref host, port } => IdleClient::connect(format!("{}:{}", host, port)) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?, }; - idle_client.subscribe(&cfg.commands_chan).await?; + idle_client + .subscribe(&cfg.commands_chan) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; let mut hup = signal(SignalKind::hangup()).unwrap(); let mut kill = signal(SignalKind::terminate()).unwrap(); @@ -183,71 +229,74 @@ pub async fn mpdpopm(cfg: Config) -> std::result::Result<(), Error> { let mut idle = Box::pin(idle_client.idle().fuse()); loop { select! { - _ = ctrl_c => { - info!("got ctrl-C"); - done = true; - break; - }, - _ = sighup => { - info!("got SIGHUP"); - done = true; - break; - }, - _ = sigkill => { - info!("got SIGKILL"); - done = true; - break; - }, - _ = tick => { - tick.set(delay_for(Duration::from_millis(cfg.poll_interval_ms)).fuse()); - if let Some(fut) = state.update(&mut client, - &cfg.playcount_command, - &mut cfg.playcount_command_args - .iter() - .cloned(), - music_dir).await? { - cmds.push(fut); - } - }, - next = cmds.next() => match next { - Some(out) => { - debug!("output status is {:#?}", out.out); - match out.upd { - Some(uri) => { - debug!("{} needs to be updated", uri); - client.update(&uri).await?; - }, - None => debug!("No database update needed"), - } - }, - None => { - debug!("No more commands to process."); - } - }, - res = idle => match res { - Ok(subsys) => { - debug!("subsystem {} changed", subsys); - if subsys == IdleSubSystem::Player { + _ = ctrl_c => { + info!("got ctrl-C"); + done = true; + break; + }, + _ = sighup => { + info!("got SIGHUP"); + done = true; + break; + }, + _ = sigkill => { + info!("got SIGKILL"); + done = true; + break; + }, + _ = tick => { + tick.set(delay_for(Duration::from_millis(cfg.poll_interval_ms)).fuse()); if let Some(fut) = state.update(&mut client, &cfg.playcount_command, &mut cfg.playcount_command_args .iter() .cloned(), - music_dir).await? { + music_dir).await.map_err(|err| Error::Playcounts{source: err, back: Backtrace::new()})? { cmds.push(fut); } - } else if subsys == IdleSubSystem::Message { - msg_check_needed = true; + }, + next = cmds.next() => match next { + Some(out) => { + debug!("output status is {:#?}", out.out); + match out.upd { + Some(uri) => { + debug!("{} needs to be updated", uri); + client.update(&uri).await.map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; + }, + None => debug!("No database update needed"), + } + }, + None => { + debug!("No more commands to process."); + } + }, + res = idle => match res { + Ok(subsys) => { + debug!("subsystem {} changed", subsys); + if subsys == IdleSubSystem::Player { + if let Some(fut) = state.update(&mut client, + &cfg.playcount_command, + &mut cfg.playcount_command_args + .iter() + .cloned(), + music_dir).await.map_err(|err| Error::Playcounts{source: err, back: Backtrace::new()})? { + cmds.push(fut); + } + } else if subsys == IdleSubSystem::Message { + msg_check_needed = true; + } + break; + }, + Err(err) => { + debug!("error {} on idle", err); + done = true; + break; + } } - break; - }, - Err(err) => { - debug!("error {} on idle", err); - done = true; - break; } - } - } } } // `idle_client' mutable borrow dropped here, which is important... diff --git a/mpdpopm/src/messages.rs b/mpdpopm/src/messages.rs index a0f45fb..405b964 100644 --- a/mpdpopm/src/messages.rs +++ b/mpdpopm/src/messages.rs @@ -28,9 +28,9 @@ //! //! The following commands are built-in: //! -//! - set rating: "rate RATING( TRACK)?" -//! - set playcount: "setpc PC( TRACK)?" -//! - set lastplayed: "setlp TIMESTAMP( TRACK)?" +//! - set rating: `rate RATING( TRACK)?` +//! - set playcount: `setpc PC( TRACK)?` +//! - set lastplayed: `setlp TIMESTAMP( TRACK)?` //! //! There is no need to provide corresponding accessors since this functionality is already provided //! via "sticker get". Dedicated accessors could provide the same functionality with slightly more @@ -39,10 +39,10 @@ //! //! I'm expanding the MPD filter functionality to include attributes tracked by mpdpopm: //! -//! - findadd replacement: "findadd FILTER [sort TYPE] [window START:END]" +//! - findadd replacement: `findadd FILTER [sort TYPE] [window START:END]` //! (cf. [here](https://www.musicpd.org/doc/html/protocol.html#the-music-database)) //! -//! - searchadd replacement: "searchadd FILTER [sort TYPE] [window START:END]" +//! - searchadd replacement: `searchadd FILTER [sort TYPE] [window START:END]` //! (cf. [here](https://www.musicpd.org/doc/html/protocol.html#the-music-database)) //! //! Additional commands may be added through the @@ -50,15 +50,14 @@ use crate::clients::{Client, IdleClient, PlayerStatus}; use crate::commands::{GeneralizedCommand, PinnedTaggedCmdFuture}; -use crate::error_from; use crate::filters::ExpressionParser; use crate::filters_ast::{evaluate, FilterStickerNames}; use crate::playcounts::{set_last_played, set_play_count}; use crate::ratings::{set_rating, RatedTrack, RatingRequest}; +use backtrace::Backtrace; use boolinator::Boolinator; use log::debug; -use snafu::{Backtrace, GenerateBacktrace, OptionExt, Snafu}; use std::collections::{HashMap, VecDeque}; use std::convert::TryFrom; @@ -67,56 +66,87 @@ use std::path::PathBuf; //////////////////////////////////////////////////////////////////////////////////////////////////// -#[derive(Debug, Snafu)] +#[derive(Debug)] pub enum Error { - #[snafu(display("The path `{}' cannot be converted to a UTF-8 string", pth.display()))] - BadPath { pth: PathBuf }, - #[snafu(display("{}", msg))] - FilterParseError { msg: String }, - #[snafu(display("Invalid unquoted character in {}", c))] - InvalidChar { c: u8 }, - #[snafu(display("Missing closing quotes"))] + BadPath { + pth: PathBuf, + }, + FilterParseError { + msg: String, + }, + InvalidChar { + c: u8, + }, NoClosingQuotes, - #[snafu(display("No command specified"))] NoCommand, - #[snafu(display("`{}' is not implemented, yet", feature))] - NotImplemented { feature: String }, - #[snafu(display("{}", cause))] - Other { - #[snafu(source(true))] - cause: Box, - #[snafu(backtrace(true))] - back: Backtrace, + NotImplemented { + feature: String, }, - #[snafu(display("Can't rate the current track when the player is stopped"))] PlayerStopped, - #[snafu(display("Trailing backslash"))] TrailingBackslash, - #[snafu(display( - "We received messages for an unknown channel `{}'; this is likely a bug; please -consider filing a report to sp1ff@pobox.com", - chan - ))] UnknownChannel { chan: String, - #[snafu(backtrace(true))] back: Backtrace, }, - #[snafu(display("We received an unknown message: `{}'", name))] UnknownCommand { name: String, - #[snafu(backtrace(true))] + back: Backtrace, + }, + Client { + source: crate::clients::Error, + back: Backtrace, + }, + Ratings { + source: crate::ratings::Error, + back: Backtrace, + }, + Playcount { + source: crate::playcounts::Error, + back: Backtrace, + }, + Filter { + source: crate::filters_ast::Error, + back: Backtrace, + }, + Command { + source: crate::commands::Error, + back: Backtrace, + }, + Utf8 { + source: std::str::Utf8Error, + buf: Vec, + back: Backtrace, + }, + ExpectedInt { + source: std::num::ParseIntError, + text: String, back: Backtrace, }, } -error_from!(crate::clients::Error); -error_from!(crate::commands::Error); -error_from!(crate::filters_ast::Error); -error_from!(crate::playcounts::Error); -error_from!(crate::ratings::Error); -error_from!(std::num::ParseIntError); -error_from!(std::str::Utf8Error); +impl std::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::BadPath { pth } => write!(f, "Bad path: {:?}", pth), + Error::FilterParseError { msg } => write!(f, "Parse error: ``{}''", msg), + Error::InvalidChar { c } => write!(f, "Invalid unquoted character {}", c), + Error::NoClosingQuotes => write!(f, "Missing closing quotes"), + Error::NoCommand => write!(f, "No command specified"), + Error::NotImplemented { feature } => write!(f, "`{}' not implemented, yet", feature), + Error::PlayerStopped => write!(f, "Can't operate on the current track when the player is stopped"), + Error::TrailingBackslash => write!(f, "Trailing backslash"), + Error::UnknownChannel {chan, back: _ } => write!(f, "We received messages for an unknown channel `{}'; this is likely a bug; please consider filing a report to sp1ff@pobox.com", chan), + Error::UnknownCommand {name, back: _ } => write!(f, "We received an unknown message ``{}''", name), + Error::Client { source, back: _ } => write!(f, "Client error: {}", source), + Error::Ratings { source, back: _ } => write!(f, "Ratings eror: {}", source), + Error::Playcount { source, back: _ } => write!(f, "Playcount error: {}", source), + Error::Filter { source, back: _ } => write!(f, "Filter error: {}", source), + Error::Command { source, back: _ } => write!(f, "Command error: {}", source), + Error::Utf8 { source, buf, back: _ } => write!(f, "UTF8 error {} ({:#?})", source, buf), + Error::ExpectedInt { source, text, back: _ } => write!(f, "``{}''L {}", source, text), + } + } +} pub type Result = std::result::Result; @@ -126,7 +156,9 @@ pub type Result = std::result::Result; /// /// When a client sends a command to [mpdpopm](crate), it will look like this on the wire: /// +/// ```text /// sendmessage ${CHANNEL} "some-command \"with space\" simple \"'with single' and \\\\\"" +/// ``` /// /// In other words, the MPD "sendmessage" command takes two parameters: the channel and the /// message. The recipient (i.e. us) is responsible for breaking up the message into its constituent @@ -136,13 +168,15 @@ pub type Result = std::result::Result; /// /// 1. an un-quoted token may contain any printable ASCII character except space, tab, ' & " /// -/// 2. to include spaces, tabs, '-s or "-s, the token must be enclosed in "-s, and any "-s or \-s +/// 2. to include spaces, tabs, '-s or "-s, the token must be enclosed in "-s, and any "-s or \\-s /// therein must be backslash escaped /// /// When the messages is delivered to us, it has already been un-escaped; i.e. we will see the /// string: /// +/// ```text /// some-command "with space" simple "'with single' and \\" +/// ``` /// /// This function will break that string up into individual tokens with one more level /// of escaping removed; i.e. it will return an iterator that will yield the four tokens: @@ -150,7 +184,7 @@ pub type Result = std::result::Result; /// 1. some-command /// 2. with space /// 3. simple -/// 4. 'with single' and \ +/// 4. 'with single' and \\ /// /// [MPD](https://github.com/MusicPlayerDaemon/MPD) has a nice /// [implementation](https://github.com/MusicPlayerDaemon/MPD/blob/master/src/util/Tokenizer.cxx#L170) @@ -160,8 +194,8 @@ pub type Result = std::result::Result; /// /// Once I realized that I could split slices I saw how to implement an Iterator that do the same /// thing (an idiomatic interface to the tokenization backed by a zero-copy implementation). I was -/// inspired by "My Favorite Rust Function Signature" -/// . +/// inspired by [My Favorite Rust Function +/// Signature](). /// /// NB. This method works in terms of a slice of [`u8`] because we can't index into Strings in /// Rust, and MPD deals only in terms of ASCII at any rate. @@ -379,11 +413,18 @@ where where E: Extend, { - let m = idle_client.get_messages().await?; + let m = idle_client + .get_messages() + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; for (chan, msgs) in m { // Only supporting a single channel, ATM - (chan == command_chan).as_option().context(UnknownChannel { + (chan == command_chan).ok_or_else(|| Error::UnknownChannel { chan: String::from(chan), + back: Backtrace::new(), })?; for msg in msgs { cmds.extend(self.process(msg, client, &state, stickers).await?); @@ -425,7 +466,10 @@ where client: &mut Client, state: &PlayerStatus, ) -> Result> { - let req = RatingRequest::try_from(msg)?; + let req = RatingRequest::try_from(msg).map_err(|err| Error::Ratings { + source: err, + back: Backtrace::new(), + })?; let pathb = match req.track { RatedTrack::Current => match state { PlayerStatus::Stopped => { @@ -440,7 +484,9 @@ where }); } }; - let path: &str = pathb.to_str().context(BadPath { pth: pathb.clone() })?; + let path: &str = pathb + .to_str() + .ok_or_else(|| Error::BadPath { pth: pathb.clone() })?; debug!("Setting a rating of {} for `{}'.", req.rating, path); Ok(set_rating( @@ -452,7 +498,11 @@ where self.ratings_cmd_args.clone(), self.music_dir, ) - .await?) + .await + .map_err(|err| Error::Ratings { + source: err, + back: Backtrace::new(), + })?) } /// Handle `setpc': "PC( TRACK)?" @@ -464,8 +514,24 @@ where ) -> Result> { let text = msg.trim(); let (pc, track) = match text.find(char::is_whitespace) { - Some(idx) => (text[..idx].parse::()?, &text[idx + 1..]), - None => (text.parse::()?, ""), + Some(idx) => ( + text[..idx] + .parse::() + .map_err(|err| Error::ExpectedInt { + source: err, + text: String::from(text), + back: Backtrace::new(), + })?, + &text[idx + 1..], + ), + None => ( + text.parse::().map_err(|err| Error::ExpectedInt { + source: err, + text: String::from(text), + back: Backtrace::new(), + })?, + "", + ), }; let file = if track.is_empty() { match state { @@ -475,7 +541,7 @@ where PlayerStatus::Play(curr) | PlayerStatus::Pause(curr) => curr .file .to_str() - .context(BadPath { + .ok_or_else(|| Error::BadPath { pth: curr.file.clone(), })? .to_string(), @@ -496,7 +562,11 @@ where &mut self.playcount_cmd_args.clone(), self.music_dir, ) - .await?) + .await + .map_err(|err| Error::Playcount { + source: err, + back: Backtrace::new(), + })?) } /// Handle `setlp': "LASTPLAYED( TRACK)?" @@ -508,8 +578,24 @@ where ) -> Result> { let text = msg.trim(); let (lp, track) = match text.find(char::is_whitespace) { - Some(idx) => (text[..idx].parse::()?, &text[idx + 1..]), - None => (text.parse::()?, ""), + Some(idx) => ( + text[..idx] + .parse::() + .map_err(|err| Error::ExpectedInt { + source: err, + text: String::from(text), + back: Backtrace::new(), + })?, + &text[idx + 1..], + ), + None => ( + text.parse::().map_err(|err| Error::ExpectedInt { + source: err, + text: String::from(text), + back: Backtrace::new(), + })?, + "", + ), }; let file = if track.is_empty() { match state { @@ -519,7 +605,7 @@ where PlayerStatus::Play(curr) | PlayerStatus::Pause(curr) => curr .file .to_str() - .context(BadPath { + .ok_or_else(|| Error::BadPath { pth: curr.file.clone(), })? .to_string(), @@ -527,7 +613,12 @@ where } else { track.to_string() }; - set_last_played(client, self.lastplayed_sticker, &file, lp).await?; + set_last_played(client, self.lastplayed_sticker, &file, lp) + .await + .map_err(|err| Error::Playcount { + source: err, + back: Backtrace::new(), + })?; Ok(None) } @@ -543,7 +634,11 @@ where let mut buf = msg.into_bytes(); let args: VecDeque<&str> = tokenize(&mut buf) .map(|r| match r { - Ok(buf) => Ok(std::str::from_utf8(buf)?), + Ok(buf) => Ok(std::str::from_utf8(buf).map_err(|err| Error::Utf8 { + source: err, + buf: buf.to_vec(), + back: Backtrace::new(), + })?), Err(err) => Err(err), }) .collect::>>()?; @@ -568,7 +663,13 @@ where debug!("ast: {:#?}", ast); let mut results = Vec::new(); - for song in evaluate(&ast, true, client, stickers).await? { + for song in evaluate(&ast, true, client, stickers) + .await + .map_err(|err| Error::Filter { + source: err, + back: Backtrace::new(), + })? + { results.push(client.add(&song).await); } match results @@ -576,7 +677,10 @@ where .collect::, crate::clients::Error>>() { Ok(_) => Ok(None), - Err(err) => Err(Error::from(err)), + Err(err) => Err(Error::Client { + source: err, + back: Backtrace::new(), + }), } } @@ -592,7 +696,11 @@ where let mut buf = msg.into_bytes(); let args: VecDeque<&str> = tokenize(&mut buf) .map(|r| match r { - Ok(buf) => Ok(std::str::from_utf8(buf)?), + Ok(buf) => Ok(std::str::from_utf8(buf).map_err(|err| Error::Utf8 { + source: err, + buf: buf.to_vec(), + back: Backtrace::new(), + })?), Err(err) => Err(err), }) .collect::>>()?; @@ -617,7 +725,13 @@ where debug!("ast: {:#?}", ast); let mut results = Vec::new(); - for song in evaluate(&ast, false, client, stickers).await? { + for song in evaluate(&ast, false, client, stickers) + .await + .map_err(|err| Error::Filter { + source: err, + back: Backtrace::new(), + })? + { results.push(client.add(&song).await); } match results @@ -625,7 +739,10 @@ where .collect::, crate::clients::Error>>() { Ok(_) => Ok(None), - Err(err) => Err(Error::from(err)), + Err(err) => Err(Error::Client { + source: err, + back: Backtrace::new(), + }), } } @@ -638,7 +755,11 @@ where let mut buf = msg.into_bytes(); let mut args: VecDeque<&str> = tokenize(&mut buf) .map(|r| match r { - Ok(buf) => Ok(std::str::from_utf8(buf)?), + Ok(buf) => Ok(std::str::from_utf8(buf).map_err(|err| Error::Utf8 { + source: err, + buf: buf.to_vec(), + back: Backtrace::new(), + })?), Err(err) => Err(err), }) .collect::>>()?; @@ -652,7 +773,17 @@ where let gen_cmd = self .gen_cmds .get(cmd) - .context(UnknownCommand { name: cmd.clone() })?; - Ok(Some(gen_cmd.execute(args.iter().cloned(), &state)?)) + .ok_or_else(|| Error::UnknownCommand { + name: String::from(cmd), + back: Backtrace::new(), + })?; + Ok(Some( + gen_cmd + .execute(args.iter().cloned(), &state) + .map_err(|err| Error::Command { + source: err, + back: Backtrace::new(), + })?, + )) } } diff --git a/mpdpopm/src/playcounts.rs b/mpdpopm/src/playcounts.rs index b881359..8a0e445 100644 --- a/mpdpopm/src/playcounts.rs +++ b/mpdpopm/src/playcounts.rs @@ -17,20 +17,19 @@ //! //! # Introduction //! -//! Play counts & last played timestamps are maintained so long as [`PlayState::update`] is called +//! Play counts & last played timestamps are maintained so long as [PlayState::update] is called //! regularly (every few seconds, say). For purposes of library maintenance, however, they can be //! set explicitly: //! -//! - setpc PLAYCOUNT( TRACK)? -//! - setlp LASTPLAYED( TRACK)? +//! - `setpc PLAYCOUNT( TRACK)?` +//! - `setlp LASTPLAYED( TRACK)?` //! use crate::clients::{Client, PlayerStatus}; use crate::commands::{spawn, TaggedCommandFuture}; -use crate::error_from; +use backtrace::Backtrace; use log::{debug, info}; -use snafu::{Backtrace, GenerateBacktrace, OptionExt, Snafu}; use std::collections::HashMap; use std::path::PathBuf; @@ -40,26 +39,61 @@ use std::time::SystemTime; // Error type // //////////////////////////////////////////////////////////////////////////////////////////////////// -#[derive(Debug, Snafu)] +#[derive(Debug)] pub enum Error { - #[snafu(display("{}", cause))] - Other { - #[snafu(source(true))] - cause: Box, - #[snafu(backtrace(true))] + PlayerStopped, + BadPath { + pth: PathBuf, + }, + SystemTime { + source: std::time::SystemTimeError, + back: Backtrace, + }, + Client { + source: crate::clients::Error, + back: Backtrace, + }, + Command { + source: crate::commands::Error, back: Backtrace, }, - #[snafu(display("The current track can't be modified when the player is stopped"))] - PlayerStopped, - #[snafu(display("The path `{}' cannot be converted to a UTF-8 string", pth.display()))] - BadPath { pth: PathBuf }, } -error_from!(crate::clients::Error); -error_from!(crate::commands::Error); -error_from!(std::num::ParseFloatError); -error_from!(std::num::ParseIntError); -error_from!(std::time::SystemTimeError); +impl std::fmt::Display for Error { + #[allow(unreachable_patterns)] // the _ arm is *currently* unreachable + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::PlayerStopped => write!(f, "The MPD player is stopped"), + Error::BadPath { pth } => write!(f, "Bad path: {:?}", pth), + Error::SystemTime { source, back: _ } => { + write!(f, "Couldn't get system time: {}", source) + } + Error::Client { source, back: _ } => write!(f, "Client error: {}", source), + Error::Command { source, back: _ } => write!(f, "Command error: {}", source), + _ => write!(f, "Unknown playcount error"), + } + } +} + +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match &self { + Error::SystemTime { + ref source, + back: _, + } => Some(source), + Error::Client { + ref source, + back: _, + } => Some(source), + Error::Command { + ref source, + back: _, + } => Some(source), + _ => None, + } + } +} type Result = std::result::Result; @@ -73,7 +107,13 @@ pub async fn get_play_count( sticker: &str, file: &str, ) -> Result> { - match client.get_sticker::(file, sticker).await? { + match client + .get_sticker::(file, sticker) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? { Some(n) => Ok(Some(n)), None => Ok(None), } @@ -91,7 +131,11 @@ pub async fn set_play_count>( ) -> Result>>> { client .set_sticker(file, sticker, &format!("{}", play_count)) - .await?; + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; if cmd.len() == 0 { return Ok(None); @@ -103,7 +147,7 @@ pub async fn set_play_count>( "full-file".to_string(), full_path .to_str() - .context(BadPath { + .ok_or_else(|| Error::BadPath { pth: full_path.clone(), })? .to_string(), @@ -111,7 +155,10 @@ pub async fn set_play_count>( params.insert("playcount".to_string(), format!("{}", play_count)); Ok(Some(TaggedCommandFuture::pin( - spawn(cmd, args, ¶ms)?, + spawn(cmd, args, ¶ms).map_err(|err| Error::Command { + source: err, + back: Backtrace::new(), + })?, None, /* No need to update the DB */ ))) } @@ -122,7 +169,13 @@ pub async fn get_last_played( sticker: &str, file: &str, ) -> Result> { - Ok(client.get_sticker::(file, sticker).await?) + Ok(client + .get_sticker::(file, sticker) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?) } /// Set the last played for a track @@ -134,7 +187,11 @@ pub async fn set_last_played( ) -> Result<()> { client .set_sticker(file, sticker, &format!("{}", last_played)) - .await?; + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; Ok(()) } @@ -219,7 +276,10 @@ impl PlayState { playcount_cmd_args: &mut I, music_dir: &str, ) -> Result>>> { - let new_stat = client.status().await?; + let new_stat = client.status().await.map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; match (&self.last_server_stat, &new_stat) { (PlayerStatus::Play(last), PlayerStatus::Play(curr)) @@ -258,7 +318,7 @@ impl PlayState { curr.songid, curr.elapsed / curr.duration ); - let file = curr.file.to_str().context(BadPath { + let file = curr.file.to_str().ok_or_else(|| Error::BadPath { pth: PathBuf::from(curr.file.clone()), })?; let curr_pc = @@ -272,7 +332,11 @@ impl PlayState { &self.lastplayed_sticker, file, SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH)? + .duration_since(SystemTime::UNIX_EPOCH) + .map_err(|err| Error::SystemTime { + source: err, + back: Backtrace::new(), + })? .as_secs(), ) .await?; diff --git a/mpdpopm/src/ratings.rs b/mpdpopm/src/ratings.rs index bd59209..efcdc4e 100644 --- a/mpdpopm/src/ratings.rs +++ b/mpdpopm/src/ratings.rs @@ -17,21 +17,22 @@ //! //! # Introduction //! -//! This module contains types implementing a basic rating functionality for `mpd'. +//! This module contains types implementing a basic rating functionality for +//! [MPD](http://www.musicpd.org). //! //! # Discussion //! -//! Rating messages to the relevant channel take the form "RATING( TRACK)?" (the two components can +//! Rating messages to the relevant channel take the form `RATING( TRACK)?` (the two components can //! be separated by any whitespace). The rating can be given by an integer between 0 & 255 -//! (inclusive) represented in base ten, or as one-to-five asterisks (i.e. \*{1,5}). In the latter +//! (inclusive) represented in base ten, or as one-to-five asterisks (i.e. `\*{1,5}`). In the latter //! case, the rating will be mapped to 1-255 as per Winamp's //! [convention](http://forums.winamp.com/showpost.php?p=2903240&postcount=94): //! -//! - 224-255 :: 5 stars when READ with windows explorer, writes 255 -//! - 160-223 :: 4 stars when READ with windows explorer, writes 196 -//! - 096-159 :: 3 stars when READ with windows explorer, writes 128 -//! - 032-095 :: 2 stars when READ with windows explorer, writes 64 -//! - 001-031 :: 1 stars when READ with windows explorer, writes 1 +//! - 224-255: 5 stars when READ with windows explorer, writes 255 +//! - 160-223: 4 stars when READ with windows explorer, writes 196 +//! - 096-159: 3 stars when READ with windows explorer, writes 128 +//! - 032-095: 2 stars when READ with windows explorer, writes 64 +//! - 001-031: 1 stars when READ with windows explorer, writes 1 //! //! NB a rating of zero means "not rated". //! @@ -41,9 +42,8 @@ use crate::clients::Client; use crate::commands::{spawn, PinnedTaggedCmdFuture, TaggedCommandFuture}; -use crate::error_from; -use snafu::{Backtrace, GenerateBacktrace, OptionExt, ResultExt, Snafu}; +use backtrace::Backtrace; use std::collections::HashMap; use std::path::PathBuf; @@ -53,37 +53,62 @@ use std::path::PathBuf; //////////////////////////////////////////////////////////////////////////////////////////////////// /// An enumeration of ratings errors -#[derive(Debug, Snafu)] +#[derive(Debug)] pub enum Error { - #[snafu(display("{}", cause))] - Other { - #[snafu(source(true))] - cause: Box, - #[snafu(backtrace(true))] - back: Backtrace, - }, - /// Unable to interpret a string as a rating - #[snafu(display("Couldn't interpret `{}' as a rating", text))] - RatingError { - #[snafu(source(from(std::num::ParseIntError, Box::new)))] - cause: Box, + Rating { + source: std::num::ParseIntError, text: String, }, - #[snafu(display("Can't rate the current track when the player is stopped"))] PlayerStopped, - #[snafu(display("`{}' is not implemented, yet", feature))] - NotImplemented { feature: String }, - #[snafu(display("Path `{}' cannot be converted to a String", pth.display()))] + NotImplemented { + feature: String, + }, BadPath { pth: PathBuf, - #[snafu(backtrace(true))] back: Backtrace, }, + Client { + source: crate::clients::Error, + back: Backtrace, + }, + Command { + source: crate::commands::Error, + back: Backtrace, + }, +} + +impl std::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Error::Rating { source, text } => write!( + f, + "Unable to interpret ``{}'' as a rating: {}", + text, source + ), + Error::PlayerStopped => write!(f, "Player stopped"), + Error::NotImplemented { feature } => write!(f, "{} not implemented", feature), + Error::BadPath { pth, back: _ } => write!(f, "Bad path: {:?}", pth), + Error::Client { source, back: _ } => write!(f, "Client error: {}", source), + Error::Command { source, back: _ } => write!(f, "Command error: {}", source), + } + } } -error_from!(crate::clients::Error); -error_from!(crate::commands::Error); -error_from!(std::num::ParseIntError); +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match &self { + Error::Rating { + text: _, + ref source, + } => Some(source), + Error::Client { + ref source, + back: _, + } => Some(source), + _ => None, + } + } +} pub type Result = std::result::Result; @@ -106,11 +131,11 @@ pub struct RatingRequest { pub track: RatedTrack, } -/// Produce a RatingRequest instance from a line of `mpd' output. +/// Produce a RatingRequest instance from a line of MPD output. impl std::convert::TryFrom<&str> for RatingRequest { type Error = Error; - /// Attempt to produce a RatingRequest instance from a line of `mpd' response to a + /// Attempt to produce a RatingRequest instance from a line of MPD response to a /// "readmessages" command. After the channel line, each subsequent line will be of the form /// "message: $MESSAGE"-- this method assumes that the "message: " prefix has been stripped off /// (i.e. we're dealing with a single line of text containing only our custom message format). @@ -138,7 +163,8 @@ impl std::convert::TryFrom<&str> for RatingRequest { "****" => 196, "*****" => 255, // failing that, we try just interperting `rating' as an unsigned integer: - _ => rating.parse::().context(RatingError { + _ => rating.parse::().map_err(|err| Error::Rating { + source: err, text: String::from(rating), })?, } @@ -193,7 +219,13 @@ mod rating_request_tests { /// Retrieve the rating for a track as an unsigned int from zero to 255 pub async fn get_rating(client: &mut Client, sticker: &str, file: &str) -> Result { - match client.get_sticker::(file, sticker).await? { + match client + .get_sticker::(file, sticker) + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })? { Some(x) => Ok(x), None => Ok(0u8), } @@ -215,7 +247,11 @@ pub async fn set_rating>( ) -> Result> { client .set_sticker(file, sticker, &format!("{}", rating)) - .await?; + .await + .map_err(|err| Error::Client { + source: err, + back: Backtrace::new(), + })?; // This isn't the best way to indicate "no command"; should take an Option, instead if cmd.len() == 0 { @@ -228,15 +264,19 @@ pub async fn set_rating>( "full-file".to_string(), full_path .to_str() - .context(BadPath { + .ok_or_else(|| Error::BadPath { pth: full_path.clone(), + back: Backtrace::new(), })? .to_string(), ); params.insert("rating".to_string(), format!("{}", rating)); Ok(Some(TaggedCommandFuture::pin( - spawn(cmd, args, ¶ms)?, + spawn(cmd, args, ¶ms).map_err(|err| Error::Command { + source: err, + back: Backtrace::new(), + })?, None, /* No need to update the DB */ ))) }