use error messages from libmirage and libsndfile, code cleanup

1 files changed, 79 insertions, 25 deletions

diff --git a/miragextract.c b/miragextract.c
index 0eef029..e9d570d 100644
--- a/miragextract.c
+++ b/miragextract.c
@@ -3,6 +3,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <limits.h>
+#include <errno.h>
 
 #include <sndfile.h>
 #include <mirage/mirage.h>
@@ -14,8 +15,10 @@
 /* This should be big enough to hold any CD sector */
 #define BUF_SIZE 4096
 
+/* main() sets this to our executable name */
 const char *self;
 
+/* command-line args/opts set these */
 char *imagefile = NULL;
 char *outfilebase = "track";
 char *outfmt = "wav";
@@ -26,17 +29,35 @@ int autoname = 0;
 int want_data = 1;
 int want_audio = 1;
 
+/* CD image state */
 MirageContext *mirage = NULL;
 MirageDisc *disc = NULL;
 MirageSession *session = NULL;
 int output_track_number = 0;
 int total_bytes = 0;
 
-void die(char *msg) {
+/* libmirage errors set this */
+GError *gerr = NULL;
+
+/* print executable name: error message, then exit with failure status. */
+void die(const char *msg) {
+	if(msg) {
+		/* do nothing, use msg as-is */
+	} else if(gerr && gerr->message) {
+		/* msg was NULL, if there's a libmirage error message, use it */
+		msg = gerr->message;
+	} else {
+		/* msg was NULL and there's nothing from libmirage, fallback: */
+		msg = strerror(errno);
+	}
 	fprintf(stderr, "%s: %s\n", self, msg);
 	exit(1);
 }
 
+/* either swap bytes into a static buffer and return a pointer
+	to it, or don't swap bytes and return the original buffer.
+	Paranoia, because mirage_sector_get_data() returns a const
+	buffer (probably safe to cast away the const, but...) */
 const guint8 *apply_s_opt(const guint8 *buf, gint len) {
 	int i;
 	static guint8 newbuf[BUF_SIZE];
@@ -51,22 +72,29 @@ const guint8 *apply_s_opt(const guint8 *buf, gint len) {
 	return (const guint8 *)newbuf;
 }
 
-void print_progress(void) {
-	static char spinner[] = "-/|\\";
+/* each call to spinner() prints the next character and a backspace */
+void spinner(void) {
+	static char spinchars[] = "-/|\\";
 	static int count = 0;
-	putchar(spinner[count++]);
+
+	putchar(spinchars[count++]);
 	putchar('\b');
 	fflush(stdout);
+
 	if(count == 4) count = 0;
 }
 
+/* format human-readable quantities (567.5MB is a lot easier to read
+	than 595024576). these are 2^10 byte megabytes, as the gods intended. */
 char *human_mb(int bytes) {
 	static char buf[10];
 	sprintf(buf, "%2.1f", (float)bytes / 1048576.0);
 	return buf;
 }
 
-void extract_track(int t, int extract) {
+/* list and (optionally) extract a track. this should be broken up into
+	smaller functions for clarity. */
+void process_track(int t, int extract) {
 	MirageTrack *track;
 	MirageSector *sector;
 	int sector_type, sec = 0;
@@ -81,31 +109,32 @@ void extract_track(int t, int extract) {
 
 	printf("  %d (-t %d): ", t + 1, output_track_number);
 
-	if(!(track = mirage_session_get_track_by_index(session, t, NULL)))
-		die("can't read track");
+	if(!(track = mirage_session_get_track_by_index(session, t, &gerr)))
+		die(NULL);
 
 	sector_type = mirage_track_get_sector_type(track);
 
 	switch(sector_type) {
 		case MIRAGE_SECTOR_MODE1:
 		case MIRAGE_SECTOR_MODE2_FORM1:
-			printf("data, ");
+			printf("data: ");
 			ext = "iso";
 			if(!want_data) extract = 0;
 			break;
 		case MIRAGE_SECTOR_AUDIO:
-			printf("audio, ");
+			printf("audio: ");
 			if(!want_audio) extract = 0;
 			ext = outfmt;
 			break;
 		default:
-			printf("<unknown>, ");
+			printf("<unknown>: ");
 			ext = "raw";
 			if(!want_data) extract = 0;
 			break;
 	}
 
-	sector = mirage_track_get_sector(track, 0, 0, NULL);
+	if(!(sector = mirage_track_get_sector(track, 0, 0, &gerr)))
+		die(NULL);
 
 	sprintf(outfile, "%s%02d.%s", outfilebase, output_track_number, ext);
 
@@ -124,10 +153,13 @@ void extract_track(int t, int extract) {
 				case 'w':
 					sfi.format = SF_FORMAT_WAV | SF_FORMAT_PCM_16;
 					break;
+				default:
+					/* this should never happen */
+					break;
 			}
 
 			if(!(sfout = sf_open(outfile, SFM_WRITE, &sfi)))
-				die("can't open output audio file");
+				die(sf_strerror(sfout));
 			sf_command(sfout, SFC_SET_VBR_ENCODING_QUALITY, &quality, sizeof(double));
 
 		} else {
@@ -139,15 +171,19 @@ void extract_track(int t, int extract) {
 
 	sec = mirage_track_get_track_start(track);
 	while((sector = mirage_track_get_sector(track, sec++, 0, NULL))) {
-		mirage_sector_get_data(sector, &buf, &len, NULL);
+		if(!(mirage_sector_get_data(sector, &buf, &len, &gerr)))
+			die(NULL);
 		bytes += len;
 		if(extract) {
 			if(sfout) {
-				sf_write_short(sfout, (const short *)apply_s_opt(buf, len), len / 2);
+				int items = len / 2;
+				if(sf_write_short(sfout, (const short *)apply_s_opt(buf, len), items) != items)
+					die(sf_strerror(sfout));
 			} else {
-				fwrite(buf, len, 1, out);
+				if(fwrite(buf, len, 1, out) != 1)
+					die(strerror(errno));
 			}
-			if(isatty(1) && !(sec % 50)) print_progress();
+			if(isatty(1) && !(sec % 50)) spinner();
 		}
 		g_object_unref(sector);
 	}
@@ -162,6 +198,7 @@ void extract_track(int t, int extract) {
 	return;
 }
 
+/* assign base filename (without extension), based on image filename */
 void set_auto_name(void) {
 	static char autoname[PATH_MAX + 1];
 	char *p;
@@ -174,6 +211,17 @@ void set_auto_name(void) {
 	outfilebase = autoname;
 }
 
+/* return true if outfmt is a supported output format */
+int outfmt_ok(char *outfmt) {
+	static const char *supported[] = { "cdda", "wav", "flac", "ogg", NULL };
+	const char **p;
+
+	for(p = supported; *p; p++)
+		if(strcmp(*p, outfmt) == 0) return 1;
+
+	return 0;
+}
+
 void usage(void) {
 	char spaces[42];
 	int i, count;
@@ -193,6 +241,8 @@ void usage(void) {
 	exit(0);
 }
 
+/* this should probably use something like getopts(), but I'm trying
+	to avoid GNU extensions. */
 void parse_args(int argc, char **argv) {
 	if(argc == 1) usage();
 	if(argc > 1 && strcmp(argv[1], "--help") == 0) usage();
@@ -235,6 +285,8 @@ void parse_args(int argc, char **argv) {
 					if(nextarg) {
 						argv++, argc--;
 						outfmt = nextarg;
+						if(!outfmt_ok(outfmt))
+							die("unsupported output format for -f");
 					} else
 						die("-f option requires format argument");
 					break;
@@ -267,6 +319,8 @@ void parse_args(int argc, char **argv) {
 	if(autoname) set_auto_name();
 }
 
+/* open the disc image, iterate over the tracks, calling
+	process_track() on each one. */
 int main(int argc, char **argv) {
 	const char *p;
 	gchar *fn[2];
@@ -280,21 +334,21 @@ int main(int argc, char **argv) {
 	parse_args(argc, argv);
 
 	if(!((mirage = g_object_new(MIRAGE_TYPE_CONTEXT, NULL))))
-		die("couldn't get mirage context");
+		die("can't get mirage context");
 
-	if(!mirage_initialize(NULL))
-		die("couldn't initialize libmirage");
+	if(!mirage_initialize(&gerr))
+		die(NULL);
 
 	/* TODO: find out what image formats require multiple filenames, if any */
-	fn[0] = imagefile;
+	fn[0] = (gchar *)imagefile;
 	fn[1] = NULL;
 
-	if(!(disc = mirage_context_load_image(mirage, fn, NULL)))
-		die("couldn't load image");
+	if(!(disc = mirage_context_load_image(mirage, fn, &gerr)))
+		die(NULL);
 
 	for(s = 0; s < mirage_disc_get_number_of_sessions(disc); s++) {
-		if(!(session = mirage_disc_get_session_by_index(disc, s, NULL)))
-			die("couldn't read session");
+		if(!(session = mirage_disc_get_session_by_index(disc, s, &gerr)))
+			die(NULL);
 
 		printf("Session %d tracks:\n", s + 1);
 
@@ -305,7 +359,7 @@ int main(int argc, char **argv) {
 			if(want_track == 0 || want_track == output_track_number)
 				extract = 1;
 
-			extract_track(t, extract);
+			process_track(t, extract);
 		}
 		if(!t) printf("  (no tracks in session)\n");
 		g_object_unref(session);