Release 1.7.0

getline may return bytes in a non-ASCII compatible encoding, so it’s safer to look for the key
characters after the conversion than before.

.editorconfig

@@ -0,0 +1,8 @@
+root = true
+
+[*]
+end_of_line = lf
+insert_final_newline = true
+charset = utf-8
+ident_style = tab
+max_line_length = 100

CHANGELOG.md

@@ -1,6 +1,21 @@
 opustags changelog
 ==================
 
+1.7.0 - 2023-02-13
+------------------
+
+- Support arbitrary large OpusTags headers.
+- Handle multiline tags by prefixing their continuation lines with tabs.
+
+1.6.0 - 2021-01-01
+------------------
+
+- UTF-8 conversion errors are now fatal.
+- Introduce --raw for disabling encoding conversions.
+- Improve platform compatibility.
+
+This also happens to be opustags’s 8-year anniversary!
+
 1.5.1 - 2020-11-21
 ------------------
 

CMakeLists.txt

@@ -1,12 +1,12 @@
-cmake_minimum_required(VERSION 3.9)
+cmake_minimum_required(VERSION 3.11)
 
 project(
 	opustags
-	VERSION 1.5.1
+	VERSION 1.7.0
 	LANGUAGES CXX
 )
 
-set(CMAKE_CXX_STANDARD 17)
+set(CMAKE_CXX_STANDARD 20)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
 
 # opustags is mainly developed with glibc, which introduces a few
@@ -26,6 +26,10 @@ include(CheckIncludeFileCXX)
 check_include_file_cxx(endian.h HAVE_ENDIAN_H)
 check_include_file_cxx(sys/endian.h HAVE_SYS_ENDIAN_H)
 
+include(CheckStructHasMember)
+check_struct_has_member("struct stat" st_mtim sys/stat.h HAVE_STAT_ST_MTIM LANGUAGE CXX)
+check_struct_has_member("struct stat" st_mtimespec sys/stat.h HAVE_STAT_ST_MTIMESPEC LANGUAGE CXX)
+
 configure_file(src/config.h.in config.h @ONLY)
 include_directories(BEFORE src "${CMAKE_BINARY_DIR}" ${OGG_INCLUDE_DIRS} ${Iconv_INCLUDE_DIRS})
 

CONTRIBUTING.md

@@ -60,7 +60,6 @@ More generally, here are a few features that could be added in the future:
 - Logicial stream listing and selection for multiplexed files.
 - Escaping control characters with --escape.
 - Dump binary packets with --binary.
-- Skip encoding conversion with --raw.
 - Edition of the vendor string.
 - Edition of the arbitrary binary block past the comments.
 - Support for OpusTags packets spanning multiple pages (> 64 kB).

README.md

@@ -8,6 +8,9 @@ then edit them again to their previous values, you should get a bit-perfect copy
 file. No under-the-cover operation like writing "edited with opustags" or timestamp tagging will
 ever be performed.
 
+opustags is tag-agnostic: you can write arbitrary key-value tags, and none of them will be treated
+specially. After all, common tags like TITLE or ARTIST are nothing more than conventions.
+
 It currently has the following limitations:
 
 - The total size of all tags cannot exceed 64 kB, the maximum size of one Ogg page.
@@ -21,8 +24,8 @@ Requirements
 ------------
 
 * a POSIX-compliant system,
-* a C++17 compiler,
-* CMake ≥ 3.9,
+* a C++20 compiler,
+* CMake ≥ 3.11,
 * libogg 1.3.3.
 
 The version numbers are indicative, and it's very likely opustags will build and work fine with
@@ -62,5 +65,6 @@ Documentation
       -s, --set FIELD=VALUE         replace a comment
       -S, --set-all                 import comments from standard input
       -e, --edit                    edit tags interactively in VISUAL/EDITOR
+      --raw                         disable encoding conversion
 
 See the man page, `opustags.1`, for extensive documentation.

opustags.1

@@ -1,4 +1,4 @@
-.TH opustags 1 "December 2018" "@PROJECT_NAME@ @PROJECT_VERSION@"
+.TH opustags 1 "February 2023" "@PROJECT_NAME@ @PROJECT_VERSION@"
 .SH NAME
 opustags \- Ogg Opus tag editor
 .SH SYNOPSIS
@@ -23,7 +23,7 @@ opustags \- Ogg Opus tag editor
 It basically has two modes: read-only, and read-write for tag editing.
 .PP
 In read-only mode, only the beginning of \fIINPUT\fP is read, and the tags are
-printed on standard output.
+printed on standard output. Lines prefixed by tabs are continuation of the previous tag.
 \fIINPUT\fP can either be the name of a file or \fB-\fP to read from standard input.
 You can use the options below to edit the tags before printing them.
 This could be useful to preview some changes before writing them.
@@ -97,12 +97,21 @@ Delete all the previously existing tags.
 Sets the tags from scratch.
 All the original tags are deleted and new ones are read from standard input.
 Each line must specify a \fIFIELD=VALUE\fP pair and be separated with line feeds.
-Blank lines and lines starting with \fI#\fP are ignored.
+Empty lines and lines starting with \fI#\fP are ignored.
+Multiline tags must have their continuation lines prefixed by a single tab (in other words, every
+\fI\\n\fP must be replaced by \fI\\n\\t\fP).
 .TP
 .B \-e, \-\-edit
 Edit tags interactively by spawning the program specified by the EDITOR
 environment variable. The allowed format is the same as \fB--set-all\fP.
 If TERM and VISUAL are set, VISUAL takes precedence over EDITOR.
+.TP
+.B \-\-raw
+OpusTags metadata should always be encoded in UTF-8, as per RFC 7845. However, some files may be
+corrupted or possibly even contain intentional binary data. In that case, --raw lets you edit that
+kind of binary data without ensuring the validity of the tags encoding. This option may also be
+useful when your system encoding is different from UTF-8 and you wish to preserve the full UTF-8
+character set even though your system cannot display it.
 .SH EXAMPLES
 .PP
 List all the tags in file foo.opus:
@@ -129,13 +138,9 @@ Edit tags interactively in Vim:
 .PP
 \fBopustags\fP currently has the following limitations:
 .IP \[bu]
-The total size of all tags cannot exceed 64 kB, the maximum size of one Ogg page.
-.IP \[bu]
 Multiplexed streams are not supported.
 .IP \[bu]
-Newlines inside tags are not supported by `--set-all`.
-.IP \[bu]
-Newlines and control characters are not escaped when printing tags.
+Control characters inside tags are printed raw rather than being escaped.
 .PP
 Internally, the OpusTags packet in an Ogg Opus file may contain extra arbitrary binary data after
 the comments.  This block of data is currently not editable, but is always preserved. The same
@@ -144,6 +149,6 @@ applies for the vendor string.
 If you need a feature not currently supported, feel free to open an issue or send an email with your
 use case.
 .SH AUTHOR
-Frédéric Mangano-Tarumi <fmang+opustags@mg0.fr>
+Frédéric Mangano <fmang+opustags@mg0.fr>
 .PP
 Report bugs at <https://github.com/fmang/opustags/issues>

src/cli.cc

@@ -6,7 +6,6 @@
  * this module from the main one is to allow easy testing.
  */
 
-#include <config.h>
 #include <opustags.h>
 
 #include <errno.h>
@@ -39,6 +38,7 @@ Options:
   -s, --set FIELD=VALUE         replace a comment
   -S, --set-all                 import comments from standard input
   -e, --edit                    edit tags interactively in VISUAL/EDITOR
+  --raw                         disable encoding conversion
 
 See the man page for extensive documentation.
 )raw";
@@ -54,19 +54,20 @@ static struct option getopt_options[] = {
 	{"delete-all", no_argument, 0, 'D'},
 	{"set-all", no_argument, 0, 'S'},
 	{"edit", no_argument, 0, 'e'},
+	{"raw", no_argument, 0, 'r'},
 	{NULL, 0, 0, 0}
 };
 
-ot::status ot::parse_options(int argc, char** argv, ot::options& opt, FILE* comments_input)
+ot::options ot::parse_options(int argc, char** argv, FILE* comments_input)
 {
+	options opt;
 	static ot::encoding_converter to_utf8("", "UTF-8");
-	std::string utf8;
-	std::string::size_type equal;
+	const char* equal;
 	ot::status rc;
 	bool set_all = false;
 	opt = {};
 	if (argc == 1)
-		return {st::bad_arguments, "No arguments specified. Use -h for help."};
+		throw status {st::bad_arguments, "No arguments specified. Use -h for help."};
 	int c;
 	optind = 0;
 	while ((c = getopt_long(argc, argv, ":ho:iyd:a:s:DSe", getopt_options, NULL)) != -1) {
@@ -76,7 +77,7 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt, FILE* comm
 			break;
 		case 'o':
 			if (opt.path_out)
-				return {st::bad_arguments, "Cannot specify --output more than once."};
+				throw status {st::bad_arguments, "Cannot specify --output more than once."};
 			opt.path_out = optarg;
 			break;
 		case 'i':
@@ -87,21 +88,16 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt, FILE* comm
 			opt.overwrite = true;
 			break;
 		case 'd':
-			rc = to_utf8(optarg, strlen(optarg), utf8);
-			if (rc != ot::st::ok)
-				return {st::bad_arguments, "Could not encode argument into UTF-8: " + rc.message};
-			opt.to_delete.emplace_back(std::move(utf8));
+			opt.to_delete.emplace_back(optarg);
 			break;
 		case 'a':
 		case 's':
-			rc = to_utf8(optarg, strlen(optarg), utf8);
-			if (rc != ot::st::ok)
-				return {st::bad_arguments, "Could not encode argument into UTF-8: " + rc.message};
-			if ((equal = utf8.find('=')) == std::string::npos)
-				return {st::bad_arguments, "Comment does not contain an equal sign: "s + optarg + "."};
+			equal = strchr(optarg, '=');
+			if (equal == nullptr)
+				throw status {st::bad_arguments, "Comment does not contain an equal sign: "s + optarg + "."};
 			if (c == 's')
-				opt.to_delete.emplace_back(utf8.substr(0, equal));
-			opt.to_add.emplace_back(std::move(utf8));
+				opt.to_delete.emplace_back(optarg, equal - optarg);
+			opt.to_add.emplace_back(optarg);
 			break;
 		case 'S':
 			opt.delete_all = true;
@@ -113,16 +109,18 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt, FILE* comm
 		case 'e':
 			opt.edit_interactively = true;
 			break;
+		case 'r':
+			opt.raw = true;
+			break;
 		case ':':
-			return {st::bad_arguments,
-			        "Missing value for option '"s + argv[optind - 1] + "'."};
+			throw status {st::bad_arguments, "Missing value for option '"s + argv[optind - 1] + "'."};
 		default:
-			return {st::bad_arguments, "Unrecognized option '" +
-			        (optopt ? "-"s + static_cast<char>(optopt) : argv[optind - 1]) + "'."};
+			throw status {st::bad_arguments, "Unrecognized option '" +
+			              (optopt ? "-"s + static_cast<char>(optopt) : argv[optind - 1]) + "'."};
 		}
 	}
 	if (opt.print_help)
-		return st::ok;
+		return opt;
 
 	// All non-option arguments are input files.
 	bool stdin_as_input = false;
@@ -131,111 +129,162 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt, FILE* comm
 		opt.paths_in.emplace_back(argv[i]);
 	}
 
+	// Convert arguments to UTF-8.
+	if (!opt.raw) {
+		for (std::list<std::string>* args : { &opt.to_add, &opt.to_delete }) {
+			try {
+				for (std::string& arg : *args)
+					arg = to_utf8(arg);
+			} catch (const ot::status& rc) {
+				throw status {st::bad_arguments, "Could not encode argument into UTF-8: " + rc.message};
+			}
+		}
+	}
+
 	if (opt.in_place && opt.path_out)
-		return {st::bad_arguments, "Cannot combine --in-place and --output."};
+		throw status {st::bad_arguments, "Cannot combine --in-place and --output."};
 
 	if (opt.in_place && stdin_as_input)
-		return {st::bad_arguments, "Cannot modify standard input in place."};
+		throw status {st::bad_arguments, "Cannot modify standard input in place."};
 
 	if ((!opt.in_place || opt.edit_interactively) && opt.paths_in.size() != 1)
-		return {st::bad_arguments, "Exactly one input file must be specified."};
+		throw status {st::bad_arguments, "Exactly one input file must be specified."};
 
 	if (set_all && stdin_as_input)
-		return {st::bad_arguments, "Cannot use standard input as input file when --set-all is specified."};
+		throw status {st::bad_arguments, "Cannot use standard input as input file when --set-all is specified."};
 
 	if (opt.edit_interactively && (stdin_as_input || opt.path_out == "-"))
-		return {st::bad_arguments, "Cannot edit interactively when standard input or standard output are already used."};
+		throw status {st::bad_arguments, "Cannot edit interactively when standard input or standard output are already used."};
 
 	if (opt.edit_interactively && !opt.path_out.has_value() && !opt.in_place)
-		return {st::bad_arguments, "Cannot edit interactively when no output is specified."};
+		throw status {st::bad_arguments, "Cannot edit interactively when no output is specified."};
 
 	if (opt.edit_interactively && (opt.delete_all || !opt.to_add.empty() || !opt.to_delete.empty()))
-		return {st::bad_arguments, "Cannot mix --edit with -adDsS."};
+		throw status {st::bad_arguments, "Cannot mix --edit with -adDsS."};
 
 	if (set_all) {
 		// Read comments from stdin and prepend them to opt.to_add.
-		std::list<std::string> comments;
-		auto rc = read_comments(comments_input, comments);
-		if (rc != st::ok)
-			return rc;
+		std::list<std::string> comments = read_comments(comments_input, opt.raw);
 		opt.to_add.splice(opt.to_add.begin(), std::move(comments));
 	}
-	return st::ok;
+	return opt;
+}
+
+/** Format a UTF-8 string by adding tabulations (\t) after line feeds (\n) to mark continuation for
+ *  multiline values. */
+static std::string format_value(const std::string& source)
+{
+	auto newline_count = std::count(source.begin(), source.end(), '\n');
+
+	// General case: the value fits on a single line. Use std::string’s copy constructor for the
+	// most efficient copy we could hope for.
+	if (newline_count == 0)
+		return source;
+
+	std::string formatted;
+	formatted.reserve(source.size() + newline_count);
+	for (auto c : source) {
+		formatted.push_back(c);
+		if (c == '\n')
+			formatted.push_back('\t');
+	}
+	return formatted;
 }
 
 /**
- * \todo Find a way to support new lines such that they can be read back by #read_comment without
- *       ambiguity. We could add a raw mode and separate comments with a \0, or escape control
- *       characters with a backslash, but we should also preserve compatibiltity with potential
- *       callers that don’t escape backslashes. Maybe add options to select a mode between simple,
- *       raw, and escaped.
+ * Print comments in a human readable format that can also be read back in by #read_comment.
+ *
+ * To disambiguate between a newline embedded in a comment and a newline representing the start of
+ * the next tag, continuation lines always have a single TAB (^I) character added to the beginning.
  */
-void ot::print_comments(const std::list<std::string>& comments, FILE* output)
+void ot::print_comments(const std::list<std::string>& comments, FILE* output, bool raw)
 {
-	static ot::encoding_converter from_utf8("UTF-8", "//TRANSLIT");
+	static ot::encoding_converter from_utf8("UTF-8", "");
 	std::string local;
-	bool info_lost = false;
-	bool bad_comments = false;
-	bool has_newline = false;
 	bool has_control = false;
-	for (const std::string& comment : comments) {
-		ot::status rc = from_utf8(comment, local);
-		if (rc == ot::st::information_lost) {
-			info_lost = true;
-		} else if (rc != ot::st::ok) {
-			bad_comments = true;
-			continue;
+	for (const std::string& source_comment : comments) {
+		if (!has_control) { // Don’t bother analyzing comments if the flag is already up.
+			for (unsigned char c : source_comment) {
+				if (c < 0x20 && c != '\n') {
+					has_control = true;
+					break;
+				}
+			}
 		}
-		for (unsigned char c : comment) {
-			if (c == '\n')
-				has_newline = true;
-			else if (c < 0x20)
-				has_control = true;
+
+		std::string utf8_comment = format_value(source_comment);
+		const std::string* comment;
+		// Convert the comment from UTF-8 to the system encoding if relevant.
+		if (raw) {
+			comment = &utf8_comment;
+		} else {
+			try {
+				local = from_utf8(utf8_comment);
+				comment = &local;
+			} catch (ot::status& rc) {
+				rc.message += " See --raw.";
+				throw;
+			}
 		}
-		fwrite(local.data(), 1, local.size(), output);
+
+		fwrite(comment->data(), 1, comment->size(), output);
 		putc('\n', output);
 	}
-	if (info_lost)
-		fputs("warning: Some tags have been transliterated to your system encoding.\n", stderr);
-	if (bad_comments)
-		fputs("warning: Some tags are not properly encoded and have not been displayed.\n", stderr);
-	if (has_newline)
-		fputs("warning: Some tags contain newline characters. "
-		      "These are not supported by --set-all.\n", stderr);
 	if (has_control)
 		fputs("warning: Some tags contain control characters.\n", stderr);
 }
 
-ot::status ot::read_comments(FILE* input, std::list<std::string>& comments)
+std::list<std::string> ot::read_comments(FILE* input, bool raw)
 {
+	std::list<std::string> comments;
 	static ot::encoding_converter to_utf8("", "UTF-8");
 	comments.clear();
-	char* line = nullptr;
+	char* source_line = nullptr;
 	size_t buflen = 0;
 	ssize_t nread;
-	while ((nread = getline(&line, &buflen, input)) != -1) {
-		if (nread > 0 && line[nread - 1] == '\n')
-			--nread;
-		if (nread == 0)
-			continue;
-		if (line[0] == '#') // comment
-			continue;
-		if (memchr(line, '=', nread) == nullptr) {
-			ot::status rc = {ot::st::error, "Malformed tag: " + std::string(line, nread)};
-			free(line);
-			return rc;
-		}
-		std::string utf8;
-		ot::status rc = to_utf8(line, nread, utf8);
-		if (rc == ot::st::ok) {
-			comments.emplace_back(std::move(utf8));
+	std::string* previous_comment = nullptr;
+	while ((nread = getline(&source_line, &buflen, input)) != -1) {
+		if (nread > 0 && source_line[nread - 1] == '\n')
+			--nread; // Chomp.
+
+		std::string line;
+		if (raw) {
+			line = std::string(source_line, nread);
 		} else {
-			free(line);
-			return {ot::st::badly_encoded, "UTF-8 conversion error: " + rc.message};
+			try {
+				line = to_utf8(std::string_view(source_line, nread));
+			} catch (const ot::status& rc) {
+				free(source_line);
+				throw ot::status {ot::st::badly_encoded, "UTF-8 conversion error: " + rc.message};
+			}
+		}
+
+		if (line.empty()) {
+			// Ignore empty lines.
+			previous_comment = nullptr;
+		} else if (line[0] == '#') {
+			// Ignore comments.
+			previous_comment = nullptr;
+		} else if (line[0] == '\t') {
+			// Continuation line: append the current line to the previous tag.
+			if (previous_comment == nullptr) {
+				ot::status rc = {ot::st::error, "Unexpected continuation line: " + std::string(source_line, nread)};
+				free(source_line);
+				throw rc;
+			} else {
+				line[0] = '\n';
+				previous_comment->append(line);
+			}
+		} else if (line.find('=') == std::string::npos) {
+			ot::status rc = {ot::st::error, "Malformed tag: " + std::string(source_line, nread)};
+			free(source_line);
+			throw rc;
+		} else {
+			previous_comment = &comments.emplace_back(std::move(line));
 		}
 	}
-	free(line);
-	return ot::st::ok;
+	free(source_line);
+	return comments;
 }
 
 void ot::delete_comments(std::list<std::string>& comments, const std::string& selector)
@@ -262,7 +311,7 @@ void ot::delete_comments(std::list<std::string>& comments, const std::string& se
 }
 
 /** Apply the modifications requested by the user to the opustags packet. */
-static ot::status edit_tags(ot::opus_tags& tags, const ot::options& opt)
+static void edit_tags(ot::opus_tags& tags, const ot::options& opt)
 {
 	if (opt.delete_all) {
 		tags.comments.clear();
@@ -272,12 +321,10 @@ static ot::status edit_tags(ot::opus_tags& tags, const ot::options& opt)
 
 	for (const std::string& comment : opt.to_add)
 		tags.comments.emplace_back(comment);
-
-	return ot::st::ok;
 }
 
 /** Spawn VISUAL or EDITOR to edit the given tags. */
-static ot::status edit_tags_interactively(ot::opus_tags& tags, const std::optional<std::string>& base_path)
+static void edit_tags_interactively(ot::opus_tags& tags, const std::optional<std::string>& base_path, bool raw)
 {
 	const char* editor = nullptr;
 	if (getenv("TERM") != nullptr)
@@ -285,57 +332,59 @@ static ot::status edit_tags_interactively(ot::opus_tags& tags, const std::option
 	if (editor == nullptr) // without a terminal, or if VISUAL is unset
 		editor = getenv("EDITOR");
 	if (editor == nullptr)
-		return {ot::st::error,
-		        "No editor specified in environment variable VISUAL or EDITOR."};
+		throw ot::status {ot::st::error,
+		                 "No editor specified in environment variable VISUAL or EDITOR."};
 
 	// Building the temporary tags file.
+	ot::status rc;
 	std::string tags_path = base_path.value_or("tags") + ".XXXXXX.opustags";
 	int fd = mkstemps(const_cast<char*>(tags_path.data()), 9);
-	FILE* tags_file;
+	ot::file tags_file;
 	if (fd == -1 || (tags_file = fdopen(fd, "w")) == nullptr)
-		return {ot::st::standard_error,
-		        "Could not open '" + tags_path + "': " + strerror(errno)};
-	ot::print_comments(tags.comments, tags_file);
-	if (fclose(tags_file) != 0)
-		return {ot::st::standard_error, tags_path + ": fclose error: "s + strerror(errno)};
+		throw ot::status {ot::st::standard_error,
+		                  "Could not open '" + tags_path + "': " + strerror(errno)};
+	ot::print_comments(tags.comments, tags_file.get(), raw);
+	tags_file.reset();
 
 	// Spawn the editor, and watch the modification timestamps.
-	ot::status rc;
-	timespec before, after;
-	if ((rc = ot::get_file_timestamp(tags_path.c_str(), before)) != ot::st::ok)
-		return rc;
-	ot::status editor_rc = ot::run_editor(editor, tags_path);
-	if ((rc = ot::get_file_timestamp(tags_path.c_str(), after)) != ot::st::ok)
-		return rc; // probably because the file was deleted
+	timespec before = ot::get_file_timestamp(tags_path.c_str());
+	ot::status editor_rc;
+	try {
+		ot::run_editor(editor, tags_path);
+		editor_rc = ot::st::ok;
+	} catch (const ot::status& rc) {
+		editor_rc = rc;
+	}
+	timespec after = ot::get_file_timestamp(tags_path.c_str());
 	bool modified = (before.tv_sec != after.tv_sec || before.tv_nsec != after.tv_nsec);
 	if (editor_rc != ot::st::ok) {
 		if (modified)
 			fprintf(stderr, "warning: Leaving %s on the disk.\n", tags_path.c_str());
 		else
 			remove(tags_path.c_str());
-		return editor_rc;
+		throw editor_rc;
 	} else if (!modified) {
 		remove(tags_path.c_str());
 		fputs("Cancelling edition because the tags file was not modified.\n", stderr);
-		return ot::st::cancel;
+		throw ot::status {ot::st::cancel};
 	}
 
 	// Applying the new tags.
 	tags_file = fopen(tags_path.c_str(), "re");
 	if (tags_file == nullptr)
-		return {ot::st::standard_error, "Error opening " + tags_path + ": " + strerror(errno)};
-	if ((rc = ot::read_comments(tags_file, tags.comments)) != ot::st::ok) {
+		throw ot::status {ot::st::standard_error, "Error opening " + tags_path + ": " + strerror(errno)};
+	try {
+		tags.comments = ot::read_comments(tags_file.get(), raw);
+	} catch (const ot::status& rc) {
 		fprintf(stderr, "warning: Leaving %s on the disk.\n", tags_path.c_str());
-		return rc;
+		throw;
 	}
-	fclose(tags_file);
+	tags_file.reset();
 
 	// Remove the temporary tags file only on success, because unlike the
 	// partial Ogg file that is irrecoverable, the edited tags file
 	// contains user data, so let’s leave users a chance to recover it.
 	remove(tags_path.c_str());
-
-	return ot::st::ok;
 }
 
 /**
@@ -344,20 +393,18 @@ static ot::status edit_tags_interactively(ot::opus_tags& tags, const std::option
  *
  * The writer is optional. When writer is nullptr, opustags runs in read-only mode.
  */
-static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const ot::options &opt)
+static void process(ot::ogg_reader& reader, ot::ogg_writer* writer, const ot::options &opt)
 {
 	bool focused = false; /*< the stream on which we operate is defined */
 	int focused_serialno; /*< when focused, the serialno of the focused stream */
-	int absolute_page_no = -1; /*< page number in the physical stream, not logical */
-	for (;;) {
-		ot::status rc = reader.next_page();
-		if (rc == ot::st::end_of_stream)
-			break;
-		else if (rc == ot::st::bad_stream && absolute_page_no == -1)
-			return {ot::st::bad_stream, "Input is not a valid Ogg file."};
-		else if (rc != ot::st::ok)
-			return rc;
-		++absolute_page_no;
+
+	/** When the number of pages the OpusTags packet takes differs from the input stream to the
+	 *  output stream, we need to renumber all the succeeding pages. If the input stream
+	 *  contains gaps, the offset will naively reproduce the gaps: page numbers 0 (1) 2 4 will
+	 *  become 0 (1 2) 3 5, where (…) is the OpusTags packet, and not 0 (1 2) 3 4. */
+	int pageno_offset = 0;
+
+	while (reader.next_page()) {
 		auto serialno = ogg_page_serialno(&reader.page);
 		auto pageno = ogg_page_pageno(&reader.page);
 		if (!focused) {
@@ -365,61 +412,53 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 			focused_serialno = serialno;
 		} else if (serialno != focused_serialno) {
 			/** \todo Support mixed streams. */
-			return {ot::st::error, "Muxed streams are not supported yet."};
+			throw ot::status {ot::st::error, "Muxed streams are not supported yet."};
 		}
-		if (absolute_page_no == 0) { // Identification header
+		if (reader.absolute_page_no == 0) { // Identification header
 			if (!ot::is_opus_stream(reader.page))
-				return {ot::st::error, "Not an Opus stream."};
-			if (writer) {
-				rc = writer->write_page(reader.page);
-				if (rc != ot::st::ok)
-					return rc;
-			}
-		} else if (absolute_page_no == 1) { // Comment header
+				throw ot::status {ot::st::error, "Not an Opus stream."};
+			if (writer)
+				writer->write_page(reader.page);
+		} else if (reader.absolute_page_no == 1) { // Comment header
 			ot::opus_tags tags;
-			rc = reader.process_header_packet(
-				[&tags](ogg_packet& p) { return ot::parse_tags(p, tags); });
-			if (rc != ot::st::ok)
-				return rc;
-			if ((rc = edit_tags(tags, opt)) != ot::st::ok)
-				return rc;
+			reader.process_header_packet([&tags](ogg_packet& p) { tags = ot::parse_tags(p); });
+			edit_tags(tags, opt);
 			if (writer) {
 				if (opt.edit_interactively) {
 					fflush(writer->file); // flush before calling the subprocess
-					if ((rc = edit_tags_interactively(tags, writer->path)) != ot::st::ok)
-						return rc;
+					edit_tags_interactively(tags, writer->path, opt.raw);
 				}
 				auto packet = ot::render_tags(tags);
-				rc = writer->write_header_packet(serialno, pageno, packet);
-				if (rc != ot::st::ok)
-					return rc;
+				writer->write_header_packet(serialno, pageno, packet);
+				pageno_offset = writer->next_page_no - 1 - reader.absolute_page_no;
 			} else {
-				ot::print_comments(tags.comments, stdout);
+				ot::print_comments(tags.comments, stdout, opt.raw);
 				break;
 			}
-		} else {
-			if (writer && (rc = writer->write_page(reader.page)) != ot::st::ok)
-				return rc;
+		} else if (writer) {
+			ot::renumber_page(reader.page, pageno + pageno_offset);
+			writer->write_page(reader.page);
 		}
 	}
-	if (absolute_page_no < 1)
-		return {ot::st::error, "Expected at least 2 Ogg pages."};
-	return ot::st::ok;
+	if (reader.absolute_page_no < 1)
+		throw ot::status {ot::st::error, "Expected at least 2 Ogg pages."};
 }
 
-static ot::status run_single(const ot::options& opt, const std::string& path_in, const std::optional<std::string>& path_out)
+static void run_single(const ot::options& opt, const std::string& path_in, const std::optional<std::string>& path_out)
 {
 	ot::file input;
 	if (path_in == "-")
 		input = stdin;
 	else if ((input = fopen(path_in.c_str(), "re")) == nullptr)
-		return {ot::st::standard_error,
-		        "Could not open '" + path_in + "' for reading: " + strerror(errno)};
+		throw ot::status {ot::st::standard_error,
+		                  "Could not open '" + path_in + "' for reading: " + strerror(errno)};
 	ot::ogg_reader reader(input.get());
 
 	/* Read-only mode. */
-	if (!path_out)
-		return process(reader, nullptr, opt);
+	if (!path_out) {
+		process(reader, nullptr, opt);
+		return;
+	}
 
 	/* Read-write mode.
 	 *
@@ -444,7 +483,6 @@ static ot::status run_single(const ot::options& opt, const std::string& path_in,
 	ot::partial_file temporary_output;
 	ot::file final_output;
 
-	ot::status rc = ot::st::ok;
 	struct stat output_info;
 	if (path_out == "-") {
 		output = stdout;
@@ -453,51 +491,45 @@ static ot::status run_single(const ot::options& opt, const std::string& path_in,
 		if (!S_ISREG(output_info.st_mode)) {
 			/* Special files are opened for writing directly. */
 			if ((final_output = fopen(path_out->c_str(), "we")) == nullptr)
-				rc = {ot::st::standard_error,
-				      "Could not open '" + path_out.value() + "' for writing: " +
-				      strerror(errno)};
+				throw ot::status {ot::st::standard_error,
+				                  "Could not open '" + path_out.value() + "' for writing: " + strerror(errno)};
 			output = final_output.get();
 		} else if (opt.overwrite) {
-			rc = temporary_output.open(path_out->c_str());
+			temporary_output.open(path_out->c_str());
 			output = temporary_output.get();
 		} else {
-			rc = {ot::st::error,
-			      "'" + path_out.value() + "' already exists. Use -y to overwrite."};
+			throw ot::status {ot::st::error, "'" + path_out.value() + "' already exists. Use -y to overwrite."};
 		}
 	} else if (errno == ENOENT) {
-		rc = temporary_output.open(path_out->c_str());
+		temporary_output.open(path_out->c_str());
 		output = temporary_output.get();
 	} else {
-		rc = {ot::st::error,
-		      "Could not identify '" + path_in + "': " + strerror(errno)};
+		throw ot::status {ot::st::error, "Could not identify '" + path_in + "': " + strerror(errno)};
 	}
-	if (rc != ot::st::ok)
-		return rc;
 
 	ot::ogg_writer writer(output);
 	writer.path = path_out;
-	rc = process(reader, &writer, opt);
-	if (rc == ot::st::ok)
-		rc = temporary_output.commit();
-
-	return rc;
+	process(reader, &writer, opt);
+	temporary_output.commit();
 }
 
-ot::status ot::run(const ot::options& opt)
+void ot::run(const ot::options& opt)
 {
 	if (opt.print_help) {
 		fputs(help_message, stdout);
-		return st::ok;
+		return;
 	}
 
 	ot::status global_rc = st::ok;
 	for (const auto& path_in : opt.paths_in) {
-		ot::status rc = run_single(opt, path_in, opt.in_place ? path_in : opt.path_out);
-		if (rc != st::ok) {
+		try {
+			run_single(opt, path_in, opt.in_place ? path_in : opt.path_out);
+		} catch (const ot::status& rc) {
 			global_rc = st::error;
 			if (!rc.message.empty())
 				fprintf(stderr, "%s: error: %s\n", path_in.c_str(), rc.message.c_str());
 		}
 	}
-	return global_rc;
+	if (global_rc != st::ok)
+		throw global_rc;
 }

src/config.h.in

@@ -3,3 +3,5 @@
 
 #cmakedefine HAVE_ENDIAN_H @HAVE_ENDIAN_H@
 #cmakedefine HAVE_SYS_ENDIAN_H @HAVE_SYS_ENDIAN_H@
+#cmakedefine HAVE_STAT_ST_MTIM @HAVE_STAT_ST_MTIM@
+#cmakedefine HAVE_STAT_ST_MTIMESPEC @HAVE_STAT_ST_MTIMESPEC@

src/ogg.cc

@@ -24,93 +24,116 @@ bool ot::is_opus_stream(const ogg_page& identification_header)
 	return (memcmp(identification_header.body, "OpusHead", 8) == 0);
 }
 
-ot::status ot::ogg_reader::next_page()
+bool ot::ogg_reader::next_page()
 {
 	int rc;
 	while ((rc = ogg_sync_pageout(&sync, &page)) != 1) {
-		if (rc == -1)
-			return {st::bad_stream, "Unsynced data in stream."};
+		if (rc == -1) {
+			throw status {st::bad_stream,
+			              absolute_page_no == (size_t) -1 ? "Input is not a valid Ogg file."
+			                                              : "Unsynced data in stream."};
+		}
 		if (ogg_sync_check(&sync) != 0)
-			return {st::libogg_error, "ogg_sync_check signalled an error."};
+			throw status {st::libogg_error, "ogg_sync_check signalled an error."};
 		if (feof(file)) {
 			if (sync.fill != sync.returned)
-				return {st::bad_stream, "Unsynced data at end of stream."};
-			return {st::end_of_stream, "End of stream was reached."};
+				throw status {st::bad_stream, "Unsynced data at end of stream."};
+			return false; // end of sream
 		}
 		char* buf = ogg_sync_buffer(&sync, 65536);
 		if (buf == nullptr)
-			return {st::libogg_error, "ogg_sync_buffer failed."};
+			throw status {st::libogg_error, "ogg_sync_buffer failed."};
 		size_t len = fread(buf, 1, 65536, file);
 		if (ferror(file))
-			return {st::standard_error, "fread error: "s + strerror(errno)};
+			throw status {st::standard_error, "fread error: "s + strerror(errno)};
 		if (ogg_sync_wrote(&sync, len) != 0)
-			return {st::libogg_error, "ogg_sync_wrote failed."};
+			throw status {st::libogg_error, "ogg_sync_wrote failed."};
 	}
-	return st::ok;
+	++absolute_page_no;
+	return true;
 }
 
-ot::status ot::ogg_reader::process_header_packet(const std::function<status(ogg_packet&)>& f)
+void ot::ogg_reader::process_header_packet(const std::function<void(ogg_packet&)>& f)
 {
 	if (ogg_page_continued(&page))
-		return {ot::st::error, "Unexpected continued header page."};
+		throw status {ot::st::error, "Unexpected continued header page."};
+
+	ogg_packet packet;
 	ogg_logical_stream stream(ogg_page_serialno(&page));
 	stream.pageno = ogg_page_pageno(&page);
-	if (ogg_stream_pagein(&stream, &page) != 0)
-		return {st::libogg_error, "ogg_stream_pagein failed."};
-	ogg_packet packet;
-	int rc = ogg_stream_packetout(&stream, &packet);
-	if (ogg_stream_check(&stream) != 0 || rc == -1)
-		return {ot::st::libogg_error, "ogg_stream_packetout failed."};
-	else if (rc == 0)
-		return {ot::st::error,
-		        "Reading header packets spanning multiple pages are not yet supported. "
-		        "Please file an issue to make your wish known."};
-	ot::status f_rc = f(packet);
-	if (f_rc != ot::st::ok)
-		return f_rc;
+
+	for (;;) {
+		if (ogg_stream_pagein(&stream, &page) != 0)
+			throw status {st::libogg_error, "ogg_stream_pagein failed."};
+
+		int rc = ogg_stream_packetout(&stream, &packet);
+		if (ogg_stream_check(&stream) != 0 || rc == -1) {
+			throw status {ot::st::libogg_error, "ogg_stream_packetout failed."};
+		} else if (rc == 0) {
+			// Not enough data: read the next page.
+			if (!next_page())
+				throw status {ot::st::error, "Unterminated header packet."};
+			continue;
+		} else {
+			// The packet was successfully read.
+			break;
+		}
+	}
+
+	f(packet);
+
 	/* Ensure that there are no other segments left in the packet using the lacing state of the
 	 * stream. These are the relevant variables, as far as I understood them:
 	 *  - lacing_vals: extensible array containing the lacing values of the segments,
 	 *  - lacing_fill: number of elements in lacing_vals (not the capacity),
 	 *  - lacing_returned: index of the next segment to be processed. */
 	if (stream.lacing_returned != stream.lacing_fill)
-		return {ot::st::error, "Header page contains more than a single packet."};
-	return ot::st::ok;
+		throw status {ot::st::error, "Header page contains more than a single packet."};
 }
 
-ot::status ot::ogg_writer::write_page(const ogg_page& page)
+void ot::ogg_writer::write_page(const ogg_page& page)
 {
 	if (page.header_len < 0 || page.body_len < 0)
-		return {st::int_overflow, "Overflowing page length"};
+		throw status {st::int_overflow, "Overflowing page length"};
+
+	long pageno = ogg_page_pageno(&page);
+	if (pageno != next_page_no)
+		fprintf(stderr, "Output page number mismatch: expected %ld, got %ld.\n", next_page_no, pageno);
+	next_page_no = pageno + 1;
+
 	auto header_len = static_cast<size_t>(page.header_len);
 	auto body_len = static_cast<size_t>(page.body_len);
 	if (fwrite(page.header, 1, header_len, file) < header_len)
-		return {st::standard_error, "fwrite error: "s + strerror(errno)};
+		throw status {st::standard_error, "fwrite error: "s + strerror(errno)};
 	if (fwrite(page.body, 1, body_len, file) < body_len)
-		return {st::standard_error, "fwrite error: "s + strerror(errno)};
-	return st::ok;
+		throw status {st::standard_error, "fwrite error: "s + strerror(errno)};
 }
 
-ot::status ot::ogg_writer::write_header_packet(int serialno, int pageno, ogg_packet& packet)
+void ot::ogg_writer::write_header_packet(int serialno, int pageno, ogg_packet& packet)
 {
 	ogg_logical_stream stream(serialno);
 	stream.b_o_s = (pageno != 0);
 	stream.pageno = pageno;
 	if (ogg_stream_packetin(&stream, &packet) != 0)
-		return {ot::st::libogg_error, "ogg_stream_packetin failed"};
+		throw status {ot::st::libogg_error, "ogg_stream_packetin failed"};
+
 	ogg_page page;
-	if (ogg_stream_flush(&stream, &page) != 0) {
-		ot::status rc = write_page(page);
-		if (rc != ot::st::ok)
-			return rc;
-	} else {
-		return {ot::st::libogg_error, "ogg_stream_flush failed"};
-	}
-	if (ogg_stream_flush(&stream, &page) != 0)
-		return {ot::st::error,
-		        "Writing header packets spanning multiple pages are not yet supported. "
-		        "Please file an issue to make your wish known."};
+	while (ogg_stream_flush(&stream, &page) != 0)
+		write_page(page);
+
 	if (ogg_stream_check(&stream) != 0)
-		return {st::libogg_error, "ogg_stream_check failed"};
-	return ot::st::ok;
+		throw status {st::libogg_error, "ogg_stream_check failed"};
+}
+
+void ot::renumber_page(ogg_page& page, long new_pageno)
+{
+	// Quick optimization: don’t bother recomputing the CRC if the pageno did not change.
+	long old_pageno = ogg_page_pageno(&page);
+	if (old_pageno == new_pageno)
+		return;
+
+	/** The pageno field is located at bytes 18 to 21 (0-indexed, little-endian). */
+	uint32_t le_pageno = htole32(new_pageno);
+	memcpy(&page.header[18], &le_pageno, 4);
+	ogg_page_checksum_set(&page);
 }

src/opus.cc

@@ -25,24 +25,10 @@
 
 #include <string.h>
 
-#ifdef HAVE_ENDIAN_H
-#  include <endian.h>
-#endif
-
-#ifdef HAVE_SYS_ENDIAN_H
-#  include <sys/endian.h>
-#endif
-
-#ifdef __APPLE__
-#include <libkern/OSByteOrder.h>
-#define htole32(x) OSSwapHostToLittleInt32(x)
-#define le32toh(x) OSSwapLittleToHostInt32(x)
-#endif
-
-ot::status ot::parse_tags(const ogg_packet& packet, opus_tags& tags)
+ot::opus_tags ot::parse_tags(const ogg_packet& packet)
 {
 	if (packet.bytes < 0)
-		return {st::int_overflow, "Overflowing comment header length"};
+		throw status {st::int_overflow, "Overflowing comment header length"};
 	size_t size = static_cast<size_t>(packet.bytes);
 	const char* data = reinterpret_cast<char*>(packet.packet);
 	size_t pos = 0;
@@ -50,36 +36,36 @@ ot::status ot::parse_tags(const ogg_packet& packet, opus_tags& tags)
 
 	// Magic number
 	if (8 > size)
-		return {st::cut_magic_number, "Comment header too short for the magic number"};
+		throw status {st::cut_magic_number, "Comment header too short for the magic number"};
 	if (memcmp(data, "OpusTags", 8) != 0)
-		return {st::bad_magic_number, "Comment header did not start with OpusTags"};
+		throw status {st::bad_magic_number, "Comment header did not start with OpusTags"};
 
 	// Vendor
 	pos = 8;
 	if (pos + 4 > size)
-		return {st::cut_vendor_length,
+		throw status {st::cut_vendor_length,
 		        "Vendor string length did not fit the comment header"};
 	size_t vendor_length = le32toh(*((uint32_t*) (data + pos)));
 	if (pos + 4 + vendor_length > size)
-		return {st::cut_vendor_data, "Vendor string did not fit the comment header"};
+		throw status {st::cut_vendor_data, "Vendor string did not fit the comment header"};
 	my_tags.vendor = std::string(data + pos + 4, vendor_length);
 	pos += 4 + my_tags.vendor.size();
 
 	// Comment count
 	if (pos + 4 > size)
-		return {st::cut_comment_count, "Comment count did not fit the comment header"};
+		throw status {st::cut_comment_count, "Comment count did not fit the comment header"};
 	uint32_t count = le32toh(*((uint32_t*) (data + pos)));
 	pos += 4;
 
 	// Comments' data
 	for (uint32_t i = 0; i < count; ++i) {
 		if (pos + 4 > size)
-			return {st::cut_comment_length,
-			        "Comment length did not fit the comment header"};
+			throw status {st::cut_comment_length,
+			              "Comment length did not fit the comment header"};
 		uint32_t comment_length = le32toh(*((uint32_t*) (data + pos)));
 		if (pos + 4 + comment_length > size)
-			return {st::cut_comment_data,
-			        "Comment string did not fit the comment header"};
+			throw status {st::cut_comment_data,
+			              "Comment string did not fit the comment header"};
 		const char *comment_value = data + pos + 4;
 		my_tags.comments.emplace_back(comment_value, comment_length);
 		pos += 4 + comment_length;
@@ -88,8 +74,7 @@ ot::status ot::parse_tags(const ogg_packet& packet, opus_tags& tags)
 	// Extra data
 	my_tags.extra_data = std::string(data + pos, size - pos);
 
-	tags = std::move(my_tags);
-	return st::ok;
+	return my_tags;
 }
 
 ot::dynamic_ogg_packet ot::render_tags(const opus_tags& tags)

src/opustags.cc

@@ -15,13 +15,14 @@
  * Does practically nothing but call the cli module.
  */
 int main(int argc, char** argv) {
-	setlocale(LC_ALL, "");
-	ot::options opt;
-	ot::status rc = ot::parse_options(argc, argv, opt, stdin);
-	if (rc == ot::st::ok)
-		rc = ot::run(opt);
-	else if (!rc.message.empty())
-		fprintf(stderr, "error: %s\n", rc.message.c_str());
-
-	return rc == ot::st::ok ? EXIT_SUCCESS : EXIT_FAILURE;
+	try {
+		setlocale(LC_ALL, "");
+		ot::options opt = ot::parse_options(argc, argv, stdin);
+		ot::run(opt);
+		return 0;
+	} catch (const ot::status& rc) {
+		if (!rc.message.empty())
+			fprintf(stderr, "error: %s\n", rc.message.c_str());
+		return rc == ot::st::bad_arguments ? 2 : 1;
+	}
 }

src/opustags.h

@@ -24,6 +24,8 @@
 
 #pragma once
 
+#include <config.h>
+
 #include <iconv.h>
 #include <ogg/ogg.h>
 #include <stdio.h>
@@ -37,16 +39,26 @@
 #include <string_view>
 #include <vector>
 
+#ifdef HAVE_ENDIAN_H
+#  include <endian.h>
+#endif
+
+#ifdef HAVE_SYS_ENDIAN_H
+#  include <sys/endian.h>
+#endif
+
+#ifdef __APPLE__
+#include <libkern/OSByteOrder.h>
+#define htole32(x) OSSwapHostToLittleInt32(x)
+#define le32toh(x) OSSwapLittleToHostInt32(x)
+#endif
+
 namespace ot {
 
 /**
  * Possible return status code, ranging from errors to special statuses. They are usually
  * accompanied with a message with the #status structure.
  *
- * Functions that return non-ok status codes to signal special conditions like #end_of_stream should
- * have it explictly mentionned in their documentation. By default, a non-ok status should be
- * handled like an error.
- *
  * Error codes do not need to be ultra specific, and are mainly used to report special conditions to
  * the caller function. Ultimately, only the error message in the #status is shown to the user.
  *
@@ -63,11 +75,9 @@ enum class st {
 	cancel,
 	/* System */
 	badly_encoded,
-	information_lost,
 	child_process_failed,
 	/* Ogg */
 	bad_stream,
-	end_of_stream,
 	libogg_error,
 	/* Opus */
 	bad_magic_number,
@@ -83,19 +93,15 @@ enum class st {
 
 /**
  * Wraps a status code with an optional message. It is implictly converted to and from a
- * #status_code.
+ * #status_code. It may be thrown on error by any of the ot:: functions.
  *
  * All the statuses except #st::ok should be accompanied with a relevant error message, in case it
  * propagates back to the main function and is shown to the user.
- *
- * \todo Instead of being returned, it could be thrown. Most of the error handling code just let the
- *       status bubble. When we're confident about RAII, we're good to go. When we migrate, let's
- *       start from main and adapt the functions top-down.
  */
 struct status {
 	status(st code = st::ok) : code(code) {}
 	template<class T> status(st code, T&& message) : code(code), message(message) {}
-	operator st() { return code; }
+	operator st() const { return code; }
 	st code;
 	std::string message;
 };
@@ -128,9 +134,9 @@ public:
 	 * temporary file is created in the same directory as its destination in order to make the
 	 * final move operation instant.
 	 */
-	ot::status open(const char* destination);
+	void open(const char* destination);
 	/** Close then move the partial file to its final location. */
-	ot::status commit();
+	void commit();
 	/** Delete the temporary file. */
 	void abort();
 	/** Get the underlying FILE* handle. */
@@ -156,12 +162,9 @@ public:
 	~encoding_converter();
 	/**
 	 * Convert text using iconv. If the input sequence is invalid, return #st::badly_encoded and
-	 * abort the processing. If some character could not be converted perfectly, keep converting
-	 * the string and finally return #st::information_lost.
+	 * abort the processing, leaving out in an undefined state.
 	 */
-	status operator()(const std::string& in, std::string& out)
-		{ return (*this)(in.data(), in.size(), out); }
-	status operator()(const char* in, size_t n, std::string& out);
+	std::string operator()(std::string_view in);
 private:
 	iconv_t cd; /**< conversion descriptor */
 };
@@ -176,13 +179,13 @@ std::string shell_escape(std::string_view word);
  * editor is passed unescaped to the shell, and may contain CLI options.
  * path is the name of the file to edit, which will be passed as the last argument to editor.
  */
-ot::status run_editor(std::string_view editor, std::string_view path);
+void run_editor(std::string_view editor, std::string_view path);
 
 /**
  * Return the specified path’s mtime, i.e. the last data modification
  * timestamp.
  */
-ot::status get_file_timestamp(const char* path, timespec& mtime);
+timespec get_file_timestamp(const char* path);
 
 /** \} */
 
@@ -216,12 +219,8 @@ bool is_opus_stream(const ogg_page& identification_header);
 /**
  * Ogg reader, combining a FILE input, an ogg_sync_state reading the pages.
  *
- * Call #read_page repeatedly until #status::end_of_stream to consume the stream, and use #page to
- * check its content.
- *
- * \todo This class could be made more intuitive if it acted like an iterator, to be used like
- *       `for (ogg_page& page : ogg_reader(input))`, but the prerequisite for this is the ability to
- *       throw an exception on error.
+ * Call #read_page repeatedly until it returns false to consume the stream, and use #page to check
+ * its content.
  */
 struct ogg_reader {
 	/**
@@ -237,13 +236,12 @@ struct ogg_reader {
 	 */
 	~ogg_reader() { ogg_sync_clear(&sync); }
 	/**
-	 * Read the next page from the input file. The result, provided the status is #status::ok,
-	 * is made available in the #page field, is owned by the Ogg reader, and is valid until the
-	 * next call to #read_page.
+	 * Read the next page from the input file. The result is made available in the #page field,
+	 * is owned by the Ogg reader, and is valid until the next call to #read_page.
 	 *
-	 * After the last page was read, return #status::end_of_stream.
+	 * Return true if a page was read, false on end of stream.
 	 */
-	status next_page();
+	bool next_page();
 	/**
 	 * Read the single packet contained in the last page read, assuming it's a header page, and
 	 * call the function f on it. This function has no side effect, and calling it twice on the
@@ -252,7 +250,7 @@ struct ogg_reader {
 	 * It is currently limited to packets that fit on a single page, and should be later
 	 * extended to support packets spanning multiple pages.
 	 */
-	status process_header_packet(const std::function<status(ogg_packet&)>& f);
+	void process_header_packet(const std::function<void(ogg_packet&)>& f);
 	/**
 	 * Current page from the sync state.
 	 *
@@ -260,6 +258,12 @@ struct ogg_reader {
 	 * to ogg_sync_pageout, wrapped by #read_page.
 	 */
 	ogg_page page;
+	/**
+	 * Page number in the physical stream of the last read page, disregarding multiplexed
+	 * streams. The first page number is 0. When no page has been read, its value is
+	 * (size_t) -1.
+	 */
+	size_t absolute_page_no = -1;
 	/**
 	 * The file is our source of binary data. It is not integrated to libogg, so we need to
 	 * handle it ourselves.
@@ -294,12 +298,12 @@ struct ogg_writer {
 	 *
 	 * This is a basic I/O operation and does not even require libogg, or the stream.
 	 */
-	status write_page(const ogg_page& page);
+	void write_page(const ogg_page& page);
 	/**
 	 * Write a header packet and flush the page. Header packets are always placed alone on their
 	 * pages.
 	 */
-	status write_header_packet(int serialno, int pageno, ogg_packet& packet);
+	void write_header_packet(int serialno, int pageno, ogg_packet& packet);
 	/**
 	 * Output file. It should be opened in binary mode. We use it to write whole pages,
 	 * represented as a block of data and a length.
@@ -309,6 +313,11 @@ struct ogg_writer {
 	 * Path to the output file.
 	 */
 	std::optional<std::string> path;
+	/**
+	 * Custom counter for the sequential page number to be written. It allows us to detect
+	 * ogg_page_pageno mismatches and renumber the pages if needed.
+	 */
+	long next_page_no = 0;
 };
 
 /**
@@ -328,6 +337,9 @@ private:
 	std::unique_ptr<unsigned char[]> data;
 };
 
+/** Update the Ogg pageno field in the given page. The CRC is recomputed if needed. */
+void renumber_page(ogg_page& page, long new_pageno);
+
 /** \} */
 
 /***********************************************************************************************//**
@@ -373,10 +385,8 @@ struct opus_tags {
 
 /**
  * Read the given OpusTags packet and extract its content into an opus_tags object.
- *
- * On error, the tags object is left unchanged.
  */
-status parse_tags(const ogg_packet& packet, opus_tags& tags);
+opus_tags parse_tags(const ogg_packet& packet);
 
 /**
  * Serialize an #opus_tags object into an OpusTags Ogg packet.
@@ -450,7 +460,7 @@ struct options {
 	 *
 	 * Option: --delete, --set
 	 */
-	std::vector<std::string> to_delete;
+	std::list<std::string> to_delete;
 	/**
 	 * Delete all the existing comments.
 	 *
@@ -466,32 +476,36 @@ struct options {
 	 * Options: --add, --set, --set-all
 	 */
 	std::list<std::string> to_add;
+	/**
+	 * Disable encoding conversions. OpusTags are specified to always be encoded as UTF-8, but
+	 * if for some reason a specific file contains binary tags that someone would like to
+	 * extract and set as-is, encoding conversion would get in the way.
+	 */
+	bool raw = false;
 };
 
 /**
  * Parse the command-line arguments. Does not perform I/O related validations, but checks the
  * consistency of its arguments. Comments are read if necessary from the given stream.
- *
- * On error, the state of the options structure is unspecified.
  */
-status parse_options(int argc, char** argv, options& opt, FILE* comments);
+options parse_options(int argc, char** argv, FILE* comments);
 
 /**
  * Print all the comments, separated by line breaks. Since a comment may contain line breaks, this
  * output is not completely reliable, but it fits most cases.
  *
- * The comments must be encoded in UTF-8, and are converted to the system locale when printed.
+ * The comments must be encoded in UTF-8, and are converted to the system locale when printed,
+ * unless raw is true.
  *
  * The output generated is meant to be parseable by #ot::read_comments.
  */
-void print_comments(const std::list<std::string>& comments, FILE* output);
+void print_comments(const std::list<std::string>& comments, FILE* output, bool raw);
 
 /**
- * Parse the comments outputted by #ot::print_comments.
- *
- * The comments are converted from the system encoding to UTF-8, and returned as UTF-8.
+ * Parse the comments outputted by #ot::print_comments. Unless raw is true, the comments are
+ * converted from the system encoding to UTF-8, and returned as UTF-8.
  */
-status read_comments(FILE* input, std::list<std::string>& comments);
+std::list<std::string> read_comments(FILE* input, bool raw);
 
 /**
  * Remove all comments matching the specified selector, which may either be a field name or a
@@ -505,7 +519,7 @@ void delete_comments(std::list<std::string>& comments, const std::string& select
  * Main entry point to the opustags program, and pretty much the same as calling opustags from the
  * command-line.
  */
-status run(const options& opt);
+void run(const options& opt);
 
 /** \} */
 

src/system.cc

@@ -20,22 +20,20 @@
 
 using namespace std::string_literals;
 
-ot::status ot::partial_file::open(const char* destination)
+void ot::partial_file::open(const char* destination)
 {
-	abort();
 	final_name = destination;
 	temporary_name = final_name + ".XXXXXX.part";
 	int fd = mkstemps(const_cast<char*>(temporary_name.data()), 5);
 	if (fd == -1)
-		return {st::standard_error,
-		        "Could not create a partial file for '" + final_name + "': " +
-		        strerror(errno)};
+		throw status {st::standard_error,
+		              "Could not create a partial file for '" + final_name + "': " +
+		              strerror(errno)};
 	file = fdopen(fd, "w");
 	if (file == nullptr)
-		return {st::standard_error,
-		        "Could not get the partial file handle to '" + temporary_name + "': " +
-		        strerror(errno)};
-	return st::ok;
+		throw status {st::standard_error,
+		              "Could not get the partial file handle to '" + temporary_name + "': " +
+		              strerror(errno)};
 }
 
 static mode_t get_umask()
@@ -71,17 +69,16 @@ static void copy_permissions(const char* source, const char* dest)
 		fprintf(stderr, "warning: Could not set mode of %s: %s\n", dest, strerror(errno));
 }
 
-ot::status ot::partial_file::commit()
+void ot::partial_file::commit()
 {
 	if (file == nullptr)
-		return st::ok;
+		return;
 	file.reset();
 	copy_permissions(final_name.c_str(), temporary_name.c_str());
 	if (rename(temporary_name.c_str(), final_name.c_str()) == -1)
-		return {st::standard_error,
-		        "Could not move the result file '" + temporary_name + "' to '" +
-		        final_name + "': " + strerror(errno) + "."};
-	return st::ok;
+		throw status {st::standard_error,
+		              "Could not move the result file '" + temporary_name + "' to '" +
+		              final_name + "': " + strerror(errno) + "."};
 }
 
 void ot::partial_file::abort()
@@ -104,37 +101,36 @@ ot::encoding_converter::~encoding_converter()
 	iconv_close(cd);
 }
 
-ot::status ot::encoding_converter::operator()(const char* in, size_t n, std::string& out)
+std::string ot::encoding_converter::operator()(std::string_view in)
 {
 	iconv(cd, nullptr, nullptr, nullptr, nullptr);
-	out.clear();
-	out.reserve(n);
-	char* in_cursor = const_cast<char*>(in);
-	size_t in_left = n;
+	std::string out;
+	out.reserve(in.size());
+	char* in_cursor = const_cast<char*>(in.data());
+	size_t in_left = in.size();
 	constexpr size_t chunk_size = 1024;
 	char chunk[chunk_size];
-	bool lost_information = false;
 	for (;;) {
 		char *out_cursor = chunk;
 		size_t out_left = chunk_size;
 		size_t rc = iconv(cd, &in_cursor, &in_left, &out_cursor, &out_left);
-		if (rc == (size_t) -1 && errno != E2BIG)
-			return {ot::st::badly_encoded,
-			        "Could not convert string '" + std::string(in, n) + "': " +
-			        strerror(errno)};
-		if (rc != 0)
-			lost_information = true;
+
+		if (rc == (size_t) -1 && errno == E2BIG) {
+			// Loop normally.
+		} else if (rc == (size_t) -1) {
+			throw status {ot::st::badly_encoded, strerror(errno) + "."s};
+		} else if (rc != 0) {
+			throw status {ot::st::badly_encoded,
+			              "Some characters could not be converted into the target encoding."};
+		}
+
 		out.append(chunk, out_cursor - chunk);
 		if (in_cursor == nullptr)
 			break;
 		else if (in_left == 0)
 			in_cursor = nullptr;
 	}
-	if (lost_information)
-		return {ot::st::information_lost,
-		        "Some characters could not be converted into the target encoding "
-		        "in string '" + std::string(in, n) + "'."};
-	return ot::st::ok;
+	return out;
 }
 
 std::string ot::shell_escape(std::string_view word)
@@ -157,28 +153,34 @@ std::string ot::shell_escape(std::string_view word)
 	return escaped_word;
 }
 
-ot::status ot::run_editor(std::string_view editor, std::string_view path)
+void ot::run_editor(std::string_view editor, std::string_view path)
 {
 	std::string command = std::string(editor) + " " + shell_escape(path);
 	int status = system(command.c_str());
 
 	if (status == -1)
-		return {st::standard_error, "waitpid error: "s + strerror(errno)};
+		throw ot::status {st::standard_error, "waitpid error: "s + strerror(errno)};
 	else if (!WIFEXITED(status))
-		return {st::child_process_failed,
-		        "Child process did not terminate normally: "s + strerror(errno)};
+		throw ot::status {st::child_process_failed,
+		                 "Child process did not terminate normally: "s + strerror(errno)};
 	else if (WEXITSTATUS(status) != 0)
-		return {st::child_process_failed,
-		        "Child process exited with " + std::to_string(WEXITSTATUS(status))};
-
-	return st::ok;
+		throw ot::status {st::child_process_failed,
+		                  "Child process exited with " + std::to_string(WEXITSTATUS(status))};
 }
 
-ot::status ot::get_file_timestamp(const char* path, timespec& mtime)
+timespec ot::get_file_timestamp(const char* path)
 {
+	timespec mtime;
 	struct stat st;
 	if (stat(path, &st) == -1)
-		return {st::standard_error, path + ": stat error: "s + strerror(errno)};
-	mtime = st.st_mtim; // more precise than st_mtime
-	return st::ok;
+		throw status {st::standard_error, path + ": stat error: "s + strerror(errno)};
+#if defined(HAVE_STAT_ST_MTIM)
+	mtime = st.st_mtim;
+#elif defined(HAVE_STAT_ST_MTIMESPEC)
+	mtime = st.st_mtimespec;
+#else
+	mtime.tv_sec = st.st_mtime;
+	mtime.tv_nsec = st.st_mtimensec;
+#endif
+	return mtime;
 }

t/cli.cc

@@ -5,6 +5,16 @@
 
 using namespace std::literals::string_literals;
 
+static ot::status read_comments(FILE* input, std::list<std::string>& comments, bool raw)
+{
+	try {
+		comments = ot::read_comments(input, raw);
+	} catch (const ot::status& rc) {
+		return rc;
+	}
+	return ot::st::ok;
+}
+
 void check_read_comments()
 {
 	std::list<std::string> comments;
@@ -12,7 +22,7 @@ void check_read_comments()
 	{
 		std::string txt = "TITLE=a b c\n\nARTIST=X\nArtist=Y\n"s;
 		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
-		rc = ot::read_comments(input.get(), comments);
+		rc = read_comments(input.get(), comments, false);
 		if (rc != ot::st::ok)
 			throw failure("could not read comments");
 		auto&& expected = {"TITLE=a b c", "ARTIST=X", "Artist=Y"};
@@ -22,17 +32,42 @@ void check_read_comments()
 	{
 		std::string txt = "CORRUPTED=\xFF\xFF\n"s;
 		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
-		rc = ot::read_comments(input.get(), comments);
+		rc = read_comments(input.get(), comments, false);
 		if (rc != ot::st::badly_encoded)
 			throw failure("did not get the expected error reading corrupted data");
 	}
+	{
+		std::string txt = "RAW=\xFF\xFF\n"s;
+		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
+		rc = read_comments(input.get(), comments, true);
+		if (rc != ot::st::ok)
+			throw failure("could not read comments");
+		if (comments.front() != "RAW=\xFF\xFF")
+			throw failure("parsed user comments did not match expectations");
+	}
+	{
+		std::string txt = "MULTILINE=First\n\tSecond\n"s;
+		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
+		rc = read_comments(input.get(), comments, true);
+		if (rc != ot::st::ok)
+			throw failure("could not read comments");
+		if (comments.front() != "MULTILINE=First\nSecond")
+			throw failure("parsed user comments did not match expectations");
+	}
 	{
 		std::string txt = "MALFORMED\n"s;
 		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
-		rc = ot::read_comments(input.get(), comments);
+		rc = read_comments(input.get(), comments, false);
 		if (rc != ot::st::error)
 			throw failure("did not get the expected error reading malformed comments");
 	}
+	{
+		std::string txt = "\tBad"s;
+		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
+		rc = read_comments(input.get(), comments, true);
+		if (rc != ot::st::error)
+			throw failure("did not get the expected error reading bad continuation line");
+	}
 }
 
 /**
@@ -45,7 +80,12 @@ static ot::status parse_options(const std::vector<const char*>& args, ot::option
 	char* argv[argc];
 	for (int i = 0; i < argc; ++i)
 		argv[i] = strdup(args[i]);
-	ot::status rc = ot::parse_options(argc, argv, opt, comments);
+	ot::status rc = ot::st::ok;
+	try {
+		opt = ot::parse_options(argc, argv, comments);
+	} catch (const ot::status& e) {
+		rc = e;
+	}
 	for (int i = 0; i < argc; ++i)
 		free(argv[i]);
 	return rc;
@@ -71,7 +111,7 @@ void check_good_arguments()
 	opt = parse({"opustags", "x", "--output", "y", "-D", "-s", "X=Y Z", "-d", "a=b"});
 	if (opt.paths_in.size() != 1 || opt.paths_in.front() != "x" || !opt.path_out ||
 	    opt.path_out != "y" || !opt.delete_all || opt.overwrite || opt.to_delete.size() != 2 ||
-	    opt.to_delete[0] != "X" || opt.to_delete[1] != "a=b" ||
+	    opt.to_delete.front() != "X" || *std::next(opt.to_delete.begin()) != "a=b" ||
 	    opt.to_add != std::list<std::string>{"X=Y Z"})
 		throw failure("unexpected option parsing result for case #1");
 
@@ -90,6 +130,10 @@ void check_good_arguments()
 	if (opt.paths_in.size() != 1 || opt.paths_in[0] != "x" ||
 	    !opt.edit_interactively || !opt.overwrite || !opt.in_place)
 		throw failure("unexpected option parsing result for case #4");
+
+	opt = parse({"opustags", "-a", "X=\xFF", "--raw", "x"});
+	if (!opt.raw || opt.to_add.front() != "X=\xFF")
+		throw failure("--raw did not disable transcoding");
 }
 
 void check_bad_arguments()
@@ -101,7 +145,7 @@ void check_bad_arguments()
 		ot::status rc = parse_options(args, opt, input.get());
 		if (rc.code != error_code)
 			throw failure("bad error code for case " + name);
-		if (rc.message != message)
+		if (!rc.message.starts_with(message))
 			throw failure("bad error message for case " + name + ", got: " + rc.message);
 	};
 	auto error_case = [&error_code_case](std::vector<const char*> args, const char* message, const std::string& name) {
@@ -111,7 +155,6 @@ void check_bad_arguments()
 	error_case({"opustags", "-a", "X"}, "Comment does not contain an equal sign: X.", "bad comment for -a");
 	error_case({"opustags", "--set", "X"}, "Comment does not contain an equal sign: X.", "bad comment for --set");
 	error_case({"opustags", "-a"}, "Missing value for option '-a'.", "short option with missing value");
-	error_case({"opustags", "--add"}, "Missing value for option '--add'.", "long option with missing value");
 	error_case({"opustags", "-x"}, "Unrecognized option '-x'.", "unrecognized short option");
 	error_case({"opustags", "--derp"}, "Unrecognized option '--derp'.", "unrecognized long option");
 	error_case({"opustags", "-x=y"}, "Unrecognized option '-x'.", "unrecognized short option with value");
@@ -139,6 +182,15 @@ void check_bad_arguments()
 	error_case({"opustags", "--edit", "x", "-i", "-d", "X"}, "Cannot mix --edit with -adDsS.", "mixing -e and -d");
 	error_case({"opustags", "--edit", "x", "-i", "-D"}, "Cannot mix --edit with -adDsS.", "mixing -e and -D");
 	error_case({"opustags", "--edit", "x", "-i", "-S"}, "Cannot mix --edit with -adDsS.", "mixing -e and -S");
+	error_case({"opustags", "-d", "\xFF", "x"},
+	           "Could not encode argument into UTF-8:",
+	           "-d with binary data");
+	error_case({"opustags", "-a", "X=\xFF", "x"},
+	           "Could not encode argument into UTF-8:",
+	           "-a with binary data");
+	error_case({"opustags", "-s", "X=\xFF", "x"},
+	           "Could not encode argument into UTF-8:",
+	           "-s with binary data");
 }
 
 static void check_delete_comments()

t/ogg.cc

@@ -11,37 +11,27 @@ static void check_ref_ogg()
 
 	ot::ogg_reader reader(input.get());
 
-	ot::status rc = reader.next_page();
-	if (rc != ot::st::ok)
+	if (reader.next_page() != true)
 		throw failure("could not read the first page");
 	if (!ot::is_opus_stream(reader.page))
 		throw failure("failed to identify the stream as opus");
-	rc = reader.process_header_packet([](ogg_packet& p) {
+	reader.process_header_packet([](ogg_packet& p) {
 		if (p.bytes != 19)
 			throw failure("unexpected length for the first packet");
-		return ot::st::ok;
 	});
-	if (rc != ot::st::ok)
-		throw failure("could not read the first packet");
 
-	rc = reader.next_page();
-	if (rc != ot::st::ok)
+	if (reader.next_page() != true)
 		throw failure("could not read the second page");
-	rc = reader.process_header_packet([](ogg_packet& p) {
+	reader.process_header_packet([](ogg_packet& p) {
 		if (p.bytes != 62)
 			throw failure("unexpected length for the second packet");
-		return ot::st::ok;
 	});
-	if (rc != ot::st::ok)
-		throw failure("could not read the second packet");
 
 	while (!ogg_page_eos(&reader.page)) {
-		rc = reader.next_page();
-		if (rc != ot::st::ok)
+		if (reader.next_page() != true)
 			throw failure("failure reading a page");
 	}
-	rc = reader.next_page();
-	if (rc != ot::st::end_of_stream)
+	if (reader.next_page() != false)
 		throw failure("did not correctly detect the end of stream");
 }
 
@@ -67,7 +57,6 @@ static void check_memory_ogg()
 	ogg_packet second_packet = make_packet("Second");
 	std::vector<unsigned char> my_ogg(128);
 	size_t my_ogg_size;
-	ot::status rc;
 
 	{
 		ot::file output = fmemopen(my_ogg.data(), my_ogg.size(), "w");
@@ -75,11 +64,7 @@ static void check_memory_ogg()
 			throw failure("could not open the output stream");
 		ot::ogg_writer writer(output.get());
 		writer.write_header_packet(1234, 0, first_packet);
-		if (rc != ot::st::ok)
-			throw failure("could not write the first packet");
 		writer.write_header_packet(1234, 1, second_packet);
-		if (rc != ot::st::ok)
-			throw failure("could not write the second packet");
 		my_ogg_size = ftell(output.get());
 		if (my_ogg_size != 67)
 			throw failure("unexpected output size");
@@ -90,28 +75,19 @@ static void check_memory_ogg()
 		if (input == nullptr)
 			throw failure("could not open the input stream");
 		ot::ogg_reader reader(input.get());
-		rc = reader.next_page();
-		if (rc != ot::st::ok)
+		if (reader.next_page() != true)
 			throw failure("could not read the first page");
-		rc = reader.process_header_packet([&first_packet](ogg_packet &p) {
+		reader.process_header_packet([&first_packet](ogg_packet &p) {
 			if (!same_packet(p, first_packet))
 				throw failure("unexpected content in the first packet");
-			return ot::st::ok;
 		});
-		if (rc != ot::st::ok)
-			throw failure("could not read the first packet");
-		rc = reader.next_page();
-		if (rc != ot::st::ok)
+		if (reader.next_page() != true)
 			throw failure("could not read the second page");
-		rc = reader.process_header_packet([&second_packet](ogg_packet &p) {
+		reader.process_header_packet([&second_packet](ogg_packet &p) {
 			if (!same_packet(p, second_packet))
 				throw failure("unexpected content in the second packet");
-			return ot::st::ok;
 		});
-		if (rc != ot::st::ok)
-			throw failure("could not read the second packet");
-		rc = reader.next_page();
-		if (rc != ot::st::end_of_stream)
+		if (reader.next_page() != false)
 			throw failure("unexpected third page");
 	}
 }
@@ -121,9 +97,13 @@ void check_bad_stream()
 	auto err_msg = "did not detect the stream is not an ogg stream";
 	ot::file input = fmemopen((void*) err_msg, 20, "r");
 	ot::ogg_reader reader(input.get());
-	ot::status rc = reader.next_page();
-	if (rc != ot::st::bad_stream)
-		throw failure(err_msg);
+	try {
+		reader.next_page();
+		throw failure("did not raise an error");
+	} catch (const ot::status& rc) {
+		if (rc != ot::st::bad_stream)
+			throw failure(err_msg);
+	}
 }
 
 void check_identification()
@@ -159,12 +139,29 @@ void check_identification()
 		throw failure("was not the beginning of a stream");
 }
 
+void check_renumber_page()
+{
+	ot::file input = fopen("gobble.opus", "r");
+	if (input == nullptr)
+		throw failure("could not open gobble.opus");
+
+	ot::ogg_reader reader(input.get());
+	if (reader.next_page() != true)
+		throw failure("could not read the first page");
+
+	long new_pageno = 1234;
+	ot::renumber_page(reader.page, new_pageno);
+	if (ogg_page_pageno(&reader.page) != new_pageno)
+		throw failure("renumbering failed");
+}
+
 int main(int argc, char **argv)
 {
-	std::cout << "1..4\n";
+	std::cout << "1..5\n";
 	run(check_ref_ogg, "check a reference ogg stream");
 	run(check_memory_ogg, "build and check a fresh stream");
 	run(check_bad_stream, "read a non-ogg stream");
 	run(check_identification, "stream identification");
+	run(check_renumber_page, "page renumbering");
 	return 0;
 }

t/opus.cc

@@ -14,13 +14,10 @@ static const char standard_OpusTags[] =
 
 static void parse_standard()
 {
-	ot::opus_tags tags;
 	ogg_packet op;
 	op.bytes = sizeof(standard_OpusTags) - 1;
 	op.packet = (unsigned char*) standard_OpusTags;
-	auto rc = ot::parse_tags(op, tags);
-	if (rc != ot::st::ok)
-		throw failure("ot::parse_tags did not return ok");
+	ot::opus_tags tags = ot::parse_tags(op);
 	if (tags.vendor != "opustags test packet")
 		throw failure("bad vendor string");
 	if (tags.comments.size() != 2)
@@ -35,6 +32,16 @@ static void parse_standard()
 		throw failure("found mysterious padding data");
 }
 
+static ot::status try_parse_tags(const ogg_packet& packet)
+{
+	try {
+		ot::parse_tags(packet);
+		return ot::st::ok;
+	} catch (const ot::status& rc) {
+		return rc;
+	}
+}
+
 /**
  * Try parse_tags with packets that should not valid, or that might even
  * corrupt the memory. Run this one with valgrind to ensure we're not
@@ -59,43 +66,40 @@ static void parse_corrupted()
 	char* end = packet + size;
 
 	op.bytes = 7;
-	if (ot::parse_tags(op, tags) != ot::st::cut_magic_number)
+	if (try_parse_tags(op) != ot::st::cut_magic_number)
 		throw failure("did not detect the overflowing magic number");
 	op.bytes = 11;
-	if (ot::parse_tags(op, tags) != ot::st::cut_vendor_length)
+	if (try_parse_tags(op) != ot::st::cut_vendor_length)
 		throw failure("did not detect the overflowing vendor string length");
 	op.bytes = size;
 
 	header_data[0] = 'o';
-	if (ot::parse_tags(op, tags) != ot::st::bad_magic_number)
+	if (try_parse_tags(op) != ot::st::bad_magic_number)
 		throw failure("did not detect the bad magic number");
 	header_data[0] = 'O';
 
 	*vendor_length = end - vendor_string + 1;
-	if (ot::parse_tags(op, tags) != ot::st::cut_vendor_data)
+	if (try_parse_tags(op) != ot::st::cut_vendor_data)
 		throw failure("did not detect the overflowing vendor string");
 	*vendor_length = end - vendor_string - 3;
-	if (ot::parse_tags(op, tags) != ot::st::cut_comment_count)
+	if (try_parse_tags(op) != ot::st::cut_comment_count)
 		throw failure("did not detect the overflowing comment count");
 	*vendor_length = comment_count - vendor_string;
 
 	++*comment_count;
-	if (ot::parse_tags(op, tags) != ot::st::cut_comment_length)
+	if (try_parse_tags(op) != ot::st::cut_comment_length)
 		throw failure("did not detect the overflowing comment length");
 	*first_comment_length = end - first_comment_data + 1;
-	if (ot::parse_tags(op, tags) != ot::st::cut_comment_data)
+	if (try_parse_tags(op) != ot::st::cut_comment_data)
 		throw failure("did not detect the overflowing comment data");
 }
 
 static void recode_standard()
 {
-	ot::opus_tags tags;
 	ogg_packet op;
 	op.bytes = sizeof(standard_OpusTags) - 1;
 	op.packet = (unsigned char*) standard_OpusTags;
-	auto rc = ot::parse_tags(op, tags);
-	if (rc != ot::st::ok)
-		throw failure("ot::parse_tags did not return ok");
+	ot::opus_tags tags = ot::parse_tags(op);
 	auto packet = ot::render_tags(tags);
 	if (packet.b_o_s != 0)
 		throw failure("b_o_s should not be set");
@@ -113,7 +117,6 @@ static void recode_standard()
 
 static void recode_padding()
 {
-	ot::opus_tags tags;
 	std::string padded_OpusTags(standard_OpusTags, sizeof(standard_OpusTags));
 	// ^ note: padded_OpusTags ends with a null byte here
 	padded_OpusTags += "hello";
@@ -121,9 +124,7 @@ static void recode_padding()
 	op.bytes = padded_OpusTags.size();
 	op.packet = (unsigned char*) padded_OpusTags.data();
 
-	auto rc = ot::parse_tags(op, tags);
-	if (rc != ot::st::ok)
-		throw failure("ot::parse_tags did not return ok");
+	ot::opus_tags tags = ot::parse_tags(op);
 	if (tags.extra_data != "\0hello"s)
 		throw failure("corrupted extra data");
 	// recode the packet and ensure it's exactly the same

t/opustags.t

@@ -4,7 +4,7 @@ use strict;
 use warnings;
 use utf8;
 
-use Test::More tests => 47;
+use Test::More tests => 55;
 
 use Digest::MD5;
 use File::Basename;
@@ -44,7 +44,7 @@ sub opustags {
 # Tests related to the overall opustags executable, like the help message.
 # No Opus file is manipulated here.
 
-is_deeply(opustags(), ['', <<EOF, 256], 'no options is a failure');
+is_deeply(opustags(), ['', <<EOF, 512], 'no options is a failure');
 error: No arguments specified. Use -h for help.
 EOF
 
@@ -72,6 +72,7 @@ Options:
   -s, --set FIELD=VALUE         replace a comment
   -S, --set-all                 import comments from standard input
   -e, --edit                    edit tags interactively in VISUAL/EDITOR
+  --raw                         disable encoding conversion
 
 See the man page for extensive documentation.
 EOF
@@ -79,7 +80,7 @@ EOF
 is_deeply(opustags('--help'), [$expected_help, '', 0], '--help displays the help message');
 is_deeply(opustags('-h'), [$expected_help, '', 0], '-h displays the help message too');
 
-is_deeply(opustags('--derp'), ['', <<"EOF", 256], 'unrecognized option shows an error');
+is_deeply(opustags('--derp'), ['', <<"EOF", 512], 'unrecognized option shows an error');
 error: Unrecognized option '--derp'.
 EOF
 
@@ -152,20 +153,24 @@ TITLE=gobble
 EOF
 is(md5('out.opus'), '66780307a6081523dc9040f3c47b0448', 'the file did not change');
 
-is_deeply(opustags(qw(-i out.opus -a fatal=yes -a FOO -a BAR)), ['', <<'EOF', 256], 'bad tag with --add');
+is_deeply(opustags(qw(-i out.opus -a fatal=yes -a FOO -a BAR)), ['', <<'EOF', 512], 'bad tag with --add');
 error: Comment does not contain an equal sign: FOO.
 EOF
 is(md5('out.opus'), '66780307a6081523dc9040f3c47b0448', 'the file did not change');
 
-is_deeply(opustags('out.opus', '-D', '-a', "X=foo\nbar\tquux"), [<<'END_OUT', <<'END_ERR', 0], 'control characters');
-X=foo
-bar	quux
+is_deeply(opustags('out.opus', '-D', '-a', "X=foobar\tquux"), [<<'END_OUT', <<'END_ERR', 0], 'control characters');
+X=foobar	quux
 END_OUT
-warning: Some tags contain newline characters. These are not supported by --set-all.
 warning: Some tags contain control characters.
 END_ERR
 
-is_deeply(opustags(qw(-i out.opus -s fatal=yes -s FOO -s BAR)), ['', <<'EOF', 256], 'bad tag with --set');
+is_deeply(opustags('out.opus', '-D', '-a', "X=foo\n\nbar"), [<<'END_OUT', '', 0], 'newline characters');
+X=foo
+	
+	bar
+END_OUT
+
+is_deeply(opustags(qw(-i out.opus -s fatal=yes -s FOO -s BAR)), ['', <<'EOF', 512], 'bad tag with --set');
 error: Comment does not contain an equal sign: FOO.
 EOF
 is(md5('out.opus'), '66780307a6081523dc9040f3c47b0448', 'the file did not change');
@@ -255,15 +260,16 @@ unlink('muxed.ogg');
 ####################################################################################################
 # Locale
 
-my $locale = 'fr_FR.iso88591';
+my $locale = 'en_US.iso88591';
 my @all_locales = split(' ', `locale -a`);
 
 SKIP: {
-skip "locale $locale is not present", 4 unless (any { $_ eq $locale } @all_locales);
+skip "locale $locale is not present", 5 unless (any { $_ eq $locale } @all_locales);
 
 opustags(qw(gobble.opus -a TITLE=七面鳥 -a ARTIST=éàç -o out.opus -y));
 
 local $ENV{LC_ALL} = $locale;
+local $ENV{LANGUAGE} = '';
 
 is_deeply(opustags(qw(-S out.opus), {in => <<"END_IN", mode => ':raw'}), [<<"END_OUT", '', 0], 'set all in ISO-8859-1');
 T=\xef\xef\xf6
@@ -273,14 +279,16 @@ END_OUT
 
 is_deeply(opustags('-i', 'out.opus', "--add=I=\xf9\xce", {mode => ':raw'}), ['', '', 0], 'write tags in ISO-8859-1');
 
-is_deeply(opustags('out.opus', {mode => ':raw'}), [<<"END_OUT", <<'END_ERR', 0], 'read tags in ISO-8859-1');
+is_deeply(opustags('out.opus', {mode => ':raw'}), [<<"END_OUT", <<"END_ERR", 256], 'read tags in ISO-8859-1 with incompatible characters');
+encoder=Lavc58.18.100 libopus
+END_OUT
+out.opus: error: Invalid or incomplete multibyte or wide character. See --raw.
+END_ERR
+
+is_deeply(opustags(qw(out.opus -d TITLE -d ARTIST), {mode => ':raw'}), [<<"END_OUT", '', 0], 'read tags in ISO-8859-1');
 encoder=Lavc58.18.100 libopus
-TITLE=???
-ARTIST=\xe9\xe0\xe7
 I=\xf9\xce
 END_OUT
-warning: Some tags have been transliterated to your system encoding.
-END_ERR
 
 $ENV{LC_ALL} = '';
 
@@ -290,4 +298,30 @@ TITLE=七面鳥
 ARTIST=éàç
 I=ùÎ
 END_OUT
+
+unlink('out.opus');
 }
+
+####################################################################################################
+# Raw edition
+
+is_deeply(opustags(qw(-S gobble.opus -o out.opus --raw -a), "U=\xFE", {in => <<"END_IN", mode => ':raw'}), ['', '', 0], 'raw set-all with binary data');
+T=\xFF
+END_IN
+
+is_deeply(opustags(qw(out.opus --raw), { mode => ':raw' }), [<<"END_OUT", '', 0], 'raw read');
+T=\xFF
+U=\xFE
+END_OUT
+
+unlink('out.opus');
+
+####################################################################################################
+# Multiple-page tags
+
+my $big_tags = "DATA=x\n" x 15000; # > 90K, which is over the max page size of 64KiB.
+is_deeply(opustags(qw(-S gobble.opus -o out.opus), {in => $big_tags}), ['', '', 0], 'write multi-page header');
+is_deeply(opustags('out.opus'), [$big_tags, '', 0], 'read multi-page header');
+is_deeply(opustags(qw(out.opus -i -D -a), 'encoder=Lavc58.18.100 libopus'), ['', '', 0], 'shrink the header');
+is(md5('out.opus'), '111a483596ac32352fbce4d14d16abd2', 'the result is identical to the original file');
+unlink('out.opus');

t/system.cc

@@ -10,10 +10,14 @@ void check_partial_files()
 	std::string name;
 	{
 		ot::partial_file bad_tmp;
-		is(bad_tmp.open("/dev/null"), ot::st::standard_error,
-		   "opening a device as a partial file fails");
-		is(bad_tmp.open(result), ot::st::ok,
-		   "opening a regular partial file works");
+		try {
+			bad_tmp.open("/dev/null");
+			throw failure("opening a device as a partial file should fail");
+		} catch (const ot::status& rc) {
+			is(rc, ot::st::standard_error, "opening a device as a partial file fails");
+		}
+
+		bad_tmp.open(result);
 		name = bad_tmp.name();
 		if (name.size() != strlen(result) + 12 ||
 		    name.compare(0, strlen(result), result) != 0)
@@ -22,9 +26,9 @@ void check_partial_files()
 	is(access(name.c_str(), F_OK), -1, "expect the temporary file is deleted");
 
 	ot::partial_file good_tmp;
-	is(good_tmp.open(result), ot::st::ok, "open the partial file");
+	good_tmp.open(result);
 	name = good_tmp.name();
-	is(good_tmp.commit(), ot::st::ok, "commit the result file");
+	good_tmp.commit();
 	is(access(name.c_str(), F_OK), -1, "expect the temporary file is deleted");
 	is(access(result, F_OK), 0, "expect the final result file");
 	is(remove(result), 0, "remove the result file");
@@ -34,19 +38,15 @@ void check_converter()
 {
 	const char* ephemere_iso = "\xc9\x70\x68\xe9\x6d\xe8\x72\x65";
 	ot::encoding_converter to_utf8("ISO_8859-1", "UTF-8");
-	ot::encoding_converter from_utf8("UTF-8", "ISO_8859-1//TRANSLIT");
-	std::string out;
+	ot::encoding_converter from_utf8("UTF-8", "ISO_8859-1");
 
-	ot::status rc = to_utf8(ephemere_iso, out);
-	is(rc, ot::st::ok, "conversion to UTF-8 is successful");
-	is(out, "Éphémère", "conversion to UTF-8 is correct");
+	is(to_utf8(ephemere_iso), "Éphémère", "conversion to UTF-8 is correct");
+	is(from_utf8("Éphémère"), ephemere_iso, "conversion from UTF-8 is correct");
 
-	rc = from_utf8("Éphémère", out);
-	is(rc, ot::st::ok, "conversion from UTF-8 is successful");
-	is(out, ephemere_iso, "conversion from UTF-8 is correct");
-
-	rc = from_utf8("\xFF\xFF", out);
-	is(rc, ot::st::badly_encoded, "conversion from bad UTF-8 fails");
+	try {
+		from_utf8("\xFF\xFF");
+		throw failure("conversion from bad UTF-8 did not fail");
+	} catch (const ot::status&) {}
 }
 
 void check_shell_esape()

t/tap.h

@@ -30,6 +30,8 @@ static void run(F test, const char *name)
 		ok = true;
 	} catch (failure& e) {
 		std::cerr << "# fail: " << e.what() << "\n";
+	} catch (const ot::status &rc) {
+		std::cerr << "# unexpected error: " << rc.message << "\n";
 	}
 	std::cout << (ok ? "ok" : "not ok") << " - " << name << "\n";
 }