From e26f3f268ca34d1a39132c18ac33876bc722f1f5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Mangano-Tarumi?= <fmang@mg0.fr>
Date: Sun, 16 Dec 2018 12:50:18 -0500
Subject: [PATCH] error when --set-all's parsing fails

---
 src/cli.cc     | 28 ++++++++++++++--------------
 src/opustags.h |  2 +-
 t/cli.cc       | 37 +++++++++++++++++++++++++++----------
 t/opustags.t   | 10 ++--------
 4 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/src/cli.cc b/src/cli.cc
index 80964fd..fd82b28 100644
--- a/src/cli.cc
+++ b/src/cli.cc
@@ -176,14 +176,10 @@ void ot::print_comments(const std::list<std::string>& comments, FILE* output)
 		fputs("warning: Some tags contain control characters.\n", stderr);
 }
 
-/**
- * \todo Report errors to the caller, so the program exits with a failure instead of skipping tags.
- *       This could wait until we throw ot::status instead of returning them.
- */
-std::list<std::string> ot::read_comments(FILE* input)
+ot::status ot::read_comments(FILE* input, std::list<std::string>& comments)
 {
 	static ot::encoding_converter to_utf8("", "UTF-8");
-	std::list<std::string> comments;
+	comments.clear();
 	char* line = nullptr;
 	size_t buflen = 0;
 	ssize_t nread;
@@ -193,29 +189,33 @@ std::list<std::string> ot::read_comments(FILE* input)
 		if (nread == 0)
 			continue;
 		if (memchr(line, '=', nread) == nullptr) {
-			fputs("warning: skipping malformed tag\n", stderr);
-			continue;
+			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));
 		else
-			fprintf(stderr, "warning: Skipping tag with UTF-8 conversion error: %s\n", rc.message.c_str());
+			return {ot::st::badly_encoded, "UTF-8 conversion error: " + rc.message};
 	}
 	free(line);
-	return comments;
+	return ot::st::ok;
 }
 
 /** Apply the modifications requested by the user to the opustags packet. */
 static ot::status edit_tags(ot::opus_tags& tags, const ot::options& opt)
 {
-	if (opt.set_all)
-		tags.comments = ot::read_comments(stdin);
-	else if (opt.delete_all)
+	if (opt.set_all) {
+		auto rc = ot::read_comments(stdin, tags.comments);
+		if (rc != ot::st::ok)
+			return rc;
+	} else if (opt.delete_all) {
 		tags.comments.clear();
-	else for (const std::string& name : opt.to_delete)
+	} else for (const std::string& name : opt.to_delete) {
 		ot::delete_comments(tags, name.c_str());
+	}
 
 	for (const std::string& comment : opt.to_add)
 		tags.comments.emplace_back(comment);
diff --git a/src/opustags.h b/src/opustags.h
index 800f2b7..e859812 100644
--- a/src/opustags.h
+++ b/src/opustags.h
@@ -466,7 +466,7 @@ void print_comments(const std::list<std::string>& comments, FILE* output);
  *
  * The comments are converted from the system encoding to UTF-8, and returned as UTF-8.
  */
-std::list<std::string> read_comments(FILE* input);
+status read_comments(FILE* input, std::list<std::string>& comments);
 
 /**
  * Main entry point to the opustags program, and pretty much the same as calling opustags from the
diff --git a/t/cli.cc b/t/cli.cc
index 4af8820..f8e0ddb 100644
--- a/t/cli.cc
+++ b/t/cli.cc
@@ -3,19 +3,36 @@
 
 #include <string.h>
 
-const char *user_comments = R"raw(
-TITLE=a b c
-
-ARTIST=X
-Artist=Y)raw";
+using namespace std::literals::string_literals;
 
 void check_read_comments()
 {
-	ot::file input = fmemopen(const_cast<char*>(user_comments), strlen(user_comments), "r");
-	auto comments = ot::read_comments(input.get());
-	auto&& expected = {"TITLE=a b c", "ARTIST=X", "Artist=Y"};
-	if (!std::equal(comments.begin(), comments.end(), expected.begin(), expected.end()))
-		throw failure("parsed user comments did not match expectations");
+	std::list<std::string> comments;
+	ot::status rc;
+	{
+		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);
+		if (rc != ot::st::ok)
+			throw failure("could not read comments");
+		auto&& expected = {"TITLE=a b c", "ARTIST=X", "Artist=Y"};
+		if (!std::equal(comments.begin(), comments.end(), expected.begin(), expected.end()))
+			throw failure("parsed user comments did not match expectations");
+	}
+	{
+		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);
+		if (rc != ot::st::badly_encoded)
+			throw failure("did not get the expected error reading corrupted data");
+	}
+	{
+		std::string txt = "MALFORMED\n"s;
+		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
+		rc = ot::read_comments(input.get(), comments);
+		if (rc != ot::st::error)
+			throw failure("did not get the expected error reading malformed comments");
+	}
 }
 
 void check_good_arguments()
diff --git a/t/opustags.t b/t/opustags.t
index 563575b..4e1942f 100755
--- a/t/opustags.t
+++ b/t/opustags.t
@@ -175,18 +175,12 @@ A=B
 X=Z
 END_OUT
 
-is_deeply(opustags(qw(out.opus -S), {in => <<'END_IN'}), [<<'END_OUT', <<'END_ERR', 0], 'set all with bad tags');
+is_deeply(opustags(qw(out.opus -S), {in => <<'END_IN'}), [<<'END_OUT', <<'END_ERR', 256], 'set all with bad tags');
 whatever
-
-# thing
-!
 wrong=yes
 END_IN
-wrong=yes
 END_OUT
-warning: skipping malformed tag
-warning: skipping malformed tag
-warning: skipping malformed tag
+error: Malformed tag: whatever
 END_ERR
 
 sub slurp {