*fb: improve error propagation out of setup/init

A lot of errors were being conflated as ENOMEM due to the lazy
use of NULL pointer returns for errors.

This commit reworks a handful of those return paths to instead
return an errno-style int, storing the results on success at a
supplied result pointer.

It's kind of ugly, and I make some assumptions about libdrm
setting errno on failure - it too uses this lazy API of returning
NULL pointers on failure.  Hopefully errno is always set by an
underlying ioctl failing.

The SDL error API is also pretty gross, being cross-platform it
defines its own error codes so I try vaguely map these to errno
values.

I'm considering this a first approximation at fixing this up, but
there are probably bugs as I did it real fast and nasty.

It at least seems to all still work OK here in the non-error
paths I tested.  So it doesn't seem more broken than before at a
glance.

7 files changed, 156 insertions, 90 deletions

diff --git a/src/drm_fb.c b/src/drm_fb.c
index 67e37bd..a25c889 100644
--- a/src/drm_fb.c
+++ b/src/drm_fb.c
@@ -68,59 +68,55 @@ static const char * connector_type_name(uint32_t type) {
 }
 
 
-static setting_desc_t * dev_desc_generator(void *setup_context)
+static int dev_desc_generator(void *setup_context, setting_desc_t **res_desc)
 {
-	setting_desc_t	*desc = NULL;
-
-	(void) setting_desc_clone(&(setting_desc_t){
+	return  setting_desc_clone(&(setting_desc_t){
 					.name = "DRM Device Path",
 					.key = "dev",
 					.regex = "/dev/dri/card[0-9]",
 					.preferred = "/dev/dri/card0",
 					.values = NULL,
 					.annotations = NULL
-				}, &desc);
-
-	return desc;
+				}, res_desc);
 }
 
 
 /* returns a NULL-terminated array of drm connectors */
-static const char ** get_connectors(const char *dev)
+static int get_connectors(const char *dev, char ***res_connectors)
 {
 	int		counts[64] = {};	/* assuming this is big enough */
 	char		**connectors;
-	int		i, fd;
 	drmModeRes	*res;
+	int		fd;
 
 	assert(dev);
 
 	fd = open(dev, O_RDWR);
 	if (fd == -1)
-		return NULL;
+		return -errno;
 
 	res = drmModeGetResources(fd);
 	if (!res) {
 		close(fd);
 
-		return NULL;
+		return -errno;
 	}
 
 	connectors = calloc(res->count_connectors + 1, sizeof(*connectors));
 	if (!connectors) {
 		close(fd);
 
-		return NULL;
+		return -ENOMEM;
 	}
 
-	for (i = 0; i < res->count_connectors; i++) {
+	for (int i = 0; i < res->count_connectors; i++) {
 		drmModeConnector	*con;
 
 		con = drmModeGetConnector(fd, res->connectors[i]);
 		if (!con) {
 			close(fd);
 
-			return NULL;
+			return -errno;
 		}
 
 		counts[con->connector_type]++;
@@ -132,7 +128,9 @@ static const char ** get_connectors(const char *dev)
 	drmModeFreeResources(res);
 	close(fd);
 
-	return (const char **)connectors;
+	*res_connectors = connectors;
+
+	return 0;
 }
 
 
@@ -147,47 +145,50 @@ static void free_strv(const char **strv)
 }
 
 
-static setting_desc_t * connector_desc_generator(void *setup_context)
+static int connector_desc_generator(void *setup_context, setting_desc_t **res_desc)
 {
 	drm_fb_setup_t	*s = setup_context;
 	const char	**connectors;
-	setting_desc_t	*desc = NULL;
+	int		r;
 
-	connectors = get_connectors(s->dev);
-	if (!connectors)
-		return NULL;
+	assert(s);
 
-	(void) setting_desc_clone(&(setting_desc_t){
+	r = get_connectors(s->dev, (char ***)&connectors);
+	if (r < 0)
+		return r;
+
+	r = setting_desc_clone(&(setting_desc_t){
 					.name = "DRM Connector",
 					.key = "connector",
 					.regex = "[a-zA-Z0-9]+",
 					.preferred = connectors[0],
 					.values = connectors,
 					.annotations = NULL
-				}, &desc);
-
+				}, res_desc);
 	free_strv(connectors);
 
-	return desc;
+	return r;
 }
 
 
-static drmModeConnector * lookup_connector(int fd, const char *connector)
+static int lookup_connector(int fd, const char *connector, drmModeConnector **res_connector)
 {
-	int			i, counts[64] = {};	/* assuming this is big enough */
+	int			r = -ENOENT, counts[64] = {};	/* assuming this is big enough */
 	drmModeConnector	*con = NULL;
 	drmModeRes		*res;
 
 	res = drmModeGetResources(fd);
 	if (!res)
-		goto _out;
+		return -errno;
 
-	for (i = 0; i < res->count_connectors; i++) {
+	for (int i = 0; i < res->count_connectors; i++) {
 		char	*str;
 
 		con = drmModeGetConnector(fd, res->connectors[i]);
-		if (!con)
+		if (!con) {
+			r = -errno;
 			goto _out_res;
+		}
 
 		counts[con->connector_type]++;
 		asprintf(&str, "%s-%i", connector_type_name(con->connector_type), counts[con->connector_type]); /* TODO: errors */
@@ -205,16 +206,21 @@ static drmModeConnector * lookup_connector(int fd, const char *connector)
 
 _out_res:
 	drmModeFreeResources(res);
-_out:
-	return con;
+
+	if (!con)
+		return r;
+
+	*res_connector = con;
+
+	return 0;
 }
 
 
 /* returns a NULL-terminated array of drm modes for the supplied device and connector */
-static const char ** get_modes(const char *dev, const char *connector)
+static int get_modes(const char *dev, const char *connector, const char ***res_modes)
 {
 	char			**modes = NULL;
-	int			i, fd;
+	int			fd, r = 0;
 	drmModeConnector	*con;
 
 	assert(dev);
@@ -222,50 +228,55 @@ static const char ** get_modes(const char *dev, const char *connector)
 
 	fd = open(dev, O_RDWR);
 	if (fd == -1)
-		goto _out;
+		return -errno;
 
-	con = lookup_connector(fd, connector);
-	if (!con)
+	r = lookup_connector(fd, connector, &con);
+	if (r < 0)
 		goto _out_fd;
 
 	modes = calloc(con->count_modes + 1, sizeof(*modes));
-	if (!modes)
+	if (!modes) {
+		r = -ENOMEM;
 		goto _out_con;
+	}
 
-	for (i = 0; i < con->count_modes; i++)
+	for (int i = 0; i < con->count_modes; i++)
 		asprintf(&modes[i], "%s@%"PRIu32, con->modes[i].name, con->modes[i].vrefresh);
 
+	*res_modes = (const char **)modes;
+
 _out_con:
 	drmModeFreeConnector(con);
 _out_fd:
 	close(fd);
 _out:
-	return (const char **)modes;
+	return r;
 }
 
 
-static setting_desc_t * mode_desc_generator(void *setup_context)
+static int mode_desc_generator(void *setup_context, setting_desc_t **res_desc)
 {
 	drm_fb_setup_t	*s = setup_context;
-	setting_desc_t	*desc = NULL;
 	const char	**modes;
+	int		r;
 
-	modes = get_modes(s->dev, s->connector);
-	if (!modes)
-		return NULL;
+	assert(s);
 
-	(void) setting_desc_clone(&(setting_desc_t){
+	r = get_modes(s->dev, s->connector, &modes);
+	if (r < 0)
+		return r;
+
+	r = setting_desc_clone(&(setting_desc_t){
 					.name = "DRM Video Mode",
 					.key = "mode",
 					.regex = "[0-9]+[xX][0-9]+@[0-9]+",
 					.preferred = modes[0],
 					.values = modes,
 					.annotations = NULL
-				}, &desc);
-
+				}, res_desc);
 	free_strv(modes);
 
-	return desc;
+	return r;
 }
 
 
@@ -324,58 +335,79 @@ static drmModeModeInfo * lookup_mode(drmModeConnector *connector, const char *mo
 
 
 /* prepare the drm context for use with the supplied settings */
-static void * drm_fb_init(const settings_t *settings)
+static int drm_fb_init(const settings_t *settings, void **res_context)
 {
 	drm_fb_t	*c;
 	const char	*dev;
 	const char	*connector;
 	const char	*mode;
 	drmModeEncoder	*enc;
+	int		r;
 
 	assert(settings);
 
-	if (!drmAvailable())
+	if (!drmAvailable()) {
+		r = -errno;
 		goto _err;
+	}
 
 	dev = settings_get_value(settings, "dev");
-	if (!dev)
+	if (!dev) {
+		r = -EINVAL;
 		goto _err;
+	}
 
 	connector = settings_get_value(settings, "connector");
-	if (!connector)
+	if (!connector) {
+		r = -EINVAL;
 		goto _err;
+	}
 
 	mode = settings_get_value(settings, "mode");
-	if (!mode)
+	if (!mode) {
+		r = -EINVAL;
 		goto _err;
+	}
 
 	c = calloc(1, sizeof(drm_fb_t));
-	if (!c)
+	if (!c) {
+		r = -ENOMEM;
 		goto _err;
+	}
 
 	c->drm_fd = open(dev, O_RDWR);
-	if (c->drm_fd < 0)
+	if (c->drm_fd < 0) {
+		r = -errno;
 		goto _err_ctxt;
+	}
 
-	c->connector = lookup_connector(c->drm_fd, connector);
-	if (!c->connector)
+	r = lookup_connector(c->drm_fd, connector, &c->connector);
+	if (r < 0)
 		goto _err_fd;
 
 	c->mode = lookup_mode(c->connector, mode);
-	if (!c->mode)
+	if (!c->mode) {
+		r = -EINVAL;
 		goto _err_con;
+	}
 
 	enc = drmModeGetEncoder(c->drm_fd, c->connector->encoder_id);
-	if (!enc)
+	if (!enc) {
+		r = -errno;
 		goto _err_con;
+	}
 
 	c->crtc = drmModeGetCrtc(c->drm_fd, enc->crtc_id);
-	if (!c->crtc)
+	if (!c->crtc) {
+		r = -errno;
 		goto _err_enc;
+	}
 
 	drmModeFreeEncoder(enc);
 
-	return c;
+	*res_context = c;
+
+	return 0;
 
 _err_enc:
 	drmModeFreeEncoder(enc);
@@ -386,7 +418,7 @@ _err_fd:
 _err_ctxt:
 	free(c);
 _err:
-	return NULL;
+	return r;
 }
 
 
diff --git a/src/fb.c b/src/fb.c
index f7076a3..baa1322 100644
--- a/src/fb.c
+++ b/src/fb.c
@@ -259,34 +259,35 @@ void fb_free(fb_t *fb)
 
 
 /* create a new fb instance */
-fb_t * fb_new(const fb_ops_t *ops, settings_t *settings, int n_pages)
+int fb_new(const fb_ops_t *ops, settings_t *settings, int n_pages, fb_t **res_fb)
 {
 	_fb_page_t	*page;
 	fb_t		*fb;
-	int		i;
+	int		r;
 
 	assert(ops);
 	assert(ops->page_alloc);
 	assert(ops->page_free);
 	assert(ops->page_flip);
 	assert(n_pages > 1);
+	assert(res_fb);
 
 	/* XXX: page-flipping is the only supported rendering model, requiring 2+ pages. */
 	if (n_pages < 2)
-		return NULL;
+		return -EINVAL;
 
 	fb = calloc(1, sizeof(fb_t));
 	if (!fb)
-		return NULL;
+		return -ENOMEM;
 
 	fb->ops = ops;
 	if (ops->init) {
-		fb->ops_context = ops->init(settings);
-		if (!fb->ops_context)
+		r = ops->init(settings, &fb->ops_context);
+		if (r < 0)
 			goto fail;
 	}
 
-	for (i = 0; i < n_pages; i++)
+	for (int i = 0; i < n_pages; i++)
 		fb_page_new(fb);
 
 	pthread_mutex_init(&fb->ready_mutex, NULL);
@@ -295,18 +296,23 @@ fb_t * fb_new(const fb_ops_t *ops, settings_t *settings, int n_pages)
 	pthread_cond_init(&fb->inactive_cond, NULL);
 
 	page = _fb_page_get(fb);
-	if (!page)
+	if (!page) {
+		r = -ENOMEM;
 		goto fail;
+	}
 
-	if (fb_acquire(fb, page) < 0)
+	r = fb_acquire(fb, page);
+	if (r < 0)
 		goto fail;
 
-	return fb;
+	*res_fb = fb;
+
+	return r;
 
 fail:
 	fb_free(fb);
 
-	return NULL;
+	return r;
 }
 
 
diff --git a/src/fb.h b/src/fb.h
index d9914df..b06ecb8 100644
--- a/src/fb.h
+++ b/src/fb.h
@@ -34,7 +34,7 @@ typedef struct fb_page_t {
 /* Supply this struct to fb_new() with the appropriate context */
 typedef struct fb_ops_t {
 	int	(*setup)(const settings_t *settings, setting_desc_t **next);
-	void *	(*init)(const settings_t *settings);
+	int	(*init)(const settings_t *settings, void **res_context);
 	void	(*shutdown)(void *context);
 	int	(*acquire)(void *context, void *page);
 	void	(*release)(void *context);
@@ -49,7 +49,7 @@ fb_page_t * fb_page_get(fb_t *fb);
 void fb_page_put(fb_t *fb, fb_page_t *page);
 void fb_free(fb_t *fb);
 void fb_get_put_pages_count(fb_t *fb, unsigned *count);
-fb_t * fb_new(const fb_ops_t *ops, settings_t *settings, int n_pages);
+int fb_new(const fb_ops_t *ops, settings_t *settings, int n_pages, fb_t **res_fb);
 int fb_flip(fb_t *fb);
 void fb_fragment_divide(fb_fragment_t *fragment, unsigned n_fragments, fb_fragment_t fragments[]);
 int fb_fragment_slice_single(const fb_fragment_t *fragment, unsigned n_fragments, unsigned num, fb_fragment_t *res_fragment);
diff --git a/src/rototiller.c b/src/rototiller.c
index 6857e72..84898da 100644
--- a/src/rototiller.c
+++ b/src/rototiller.c
@@ -440,8 +440,8 @@ int main(int argc, const char *argv[])
 	exit_if(!(rototiller.module = rototiller_lookup_module(settings_get_key(setup.module, 0))),
 		"unable to lookup module from settings \"%s\"", settings_get_key(setup.module, 0));
 
-	exit_if(!(rototiller.fb = fb_new(fb_ops, setup.video, NUM_FB_PAGES)),
-		"unable to create fb");
+	exit_if((r = fb_new(fb_ops, setup.video, NUM_FB_PAGES, &rototiller.fb)) < 0,
+		"unable to create fb: %s", strerror(-r));
 
 	exit_if(!fps_setup(),
 		"unable to setup fps counter");
diff --git a/src/sdl_fb.c b/src/sdl_fb.c
index dfb319a..8e48329 100644
--- a/src/sdl_fb.c
+++ b/src/sdl_fb.c
@@ -1,4 +1,5 @@
 #define SDL_MAIN_HANDLED
+#include <assert.h>
 #include <SDL.h>
 #include <stdlib.h>
 #include <errno.h>
@@ -86,24 +87,43 @@ static int sdl_fb_setup(const settings_t *settings, setting_desc_t **next_settin
 	return 0;
 }
 
+static int sdl_err_to_errno(int err)
+{
+	switch (err) {
+	case SDL_ENOMEM:
+		return ENOMEM;
+	case SDL_EFREAD:
+	case SDL_EFWRITE:
+	case SDL_EFSEEK:
+		return EIO;
+	case SDL_UNSUPPORTED:
+		return ENOTSUP;
+	default:
+		return EINVAL;
+	}
+}
 
-static void * sdl_fb_init(const settings_t *settings)
+static int sdl_fb_init(const settings_t *settings, void **res_context)
 {
 	const char	*fullscreen;
 	const char	*size;
 	sdl_fb_t	*c;
+	int		r;
+
+	assert(settings);
+	assert(res_context);
 
 	fullscreen = settings_get_value(settings, "fullscreen");
 	if (!fullscreen)
-		return NULL;
+		return -EINVAL;
 
 	size = settings_get_value(settings, "size");
 	if (!size && !strcasecmp(fullscreen, "off"))
-		return NULL;
+		return -EINVAL;
 
 	c = calloc(1, sizeof(sdl_fb_t));
 	if (!c)
-		return NULL;
+		return -ENOMEM;
 
 	if (!strcasecmp(fullscreen, "on")) {
 		if (!size)
@@ -116,25 +136,29 @@ static void * sdl_fb_init(const settings_t *settings)
 		sscanf(size, "%u%*[xX]%u", &c->width, &c->height);
 
 	SDL_SetMainReady();
-	if (SDL_Init(SDL_INIT_VIDEO) < 0) {
+	r = SDL_Init(SDL_INIT_VIDEO);
+	if (r < 0) {
 		free(c);
-		return NULL;
+		return -sdl_err_to_errno(r);
 	}
 
 	if (c->flags == SDL_WINDOW_FULLSCREEN_DESKTOP) {
 		SDL_DisplayMode	mode;
 
-		if (SDL_GetDesktopDisplayMode(0, &mode) != 0) {
+		r = SDL_GetDesktopDisplayMode(0, &mode);
+		if (r != 0) {
 			SDL_Quit();
 			free(c);
-			return NULL;
+			return -sdl_err_to_errno(r);
 		}
 
 		c->width = mode.w;
 		c->height = mode.h;
 	}
 
-	return c;
+	*res_context = c;
+
+	return 0;
 }
 
 
diff --git a/src/settings.c b/src/settings.c
index aa256c5..3ba4f15 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -196,10 +196,13 @@ int settings_apply_desc_generators(const settings_t *settings, const setting_des
 		const setting_desc_generator_t	*g = &generators[i];
 		const char			*value;
 		setting_desc_t			*desc;
+		int				r;
 
-		desc = g->func(setup_context);
-		if (!desc)
-			return -ENOMEM;
+		r = g->func(setup_context, &desc);
+		if (r < 0)
+			return r;
+
+		assert(desc);
 
 		value = settings_get_value(settings, g->key);
 		if (value) {
@@ -236,6 +239,7 @@ int setting_desc_clone(const setting_desc_t *desc, setting_desc_t **res_desc)
 	assert(desc->name);
 	assert(desc->preferred);	/* XXX: require a preferred default? */
 	assert(!desc->annotations || desc->values);
+	assert(res_desc);
 
 	d = calloc(1, sizeof(setting_desc_t));
 	if (!d)
diff --git a/src/settings.h b/src/settings.h
index af47326..c538531 100644
--- a/src/settings.h
+++ b/src/settings.h
@@ -18,7 +18,7 @@ typedef struct setting_desc_t {
 typedef struct setting_desc_generator_t {
 	const char	*key;		/* key this generator applies to */
 	const char	**value_ptr;	/* where to put the value */
-	setting_desc_t *(*func)(void *setup_context);
+	int		(*func)(void *setup_context, setting_desc_t **res_desc);
 } setting_desc_generator_t;
 
 typedef struct settings_t settings_t;