Error Handling with the Single Input Single Output Pattern
Introduction
The single-input single-output is a simple pattern for procedural C code that eases:
- Graceful error handling
- Avoid code duplication
- Improve readability.
Despite this, it is quite controversial among de developer's community due to the use of the infamous goto operator. The idea in single-input single-output is simple:
Every function should have a single entry point and a single output point.
The single entry point is natural for most of us, and probably (hopefully) de 100% of the cases. It is breakable though. In some old non-standard C compilers you could jump (using a goto) to a line in another function. Other more day-to-day examples include the use of setjump or longjump.
The single output point is way more common but gladly less harmful. The simplest example being having multiple returns in a single function. This is very typical during error handling or short-circuiting based on certain conditions.
GoTo Disclaimers
First of all, this is my personal opinion. Having said that:
- I do not encourage the arbitrary use of goto jumps.
- The single-input single-output is one of the few cases in which I believe is acceptable, even beneficial.
- Yes, you can achieve the same with nested if/else structures. No, it won't be as readable and clean.
No, you shouldn't be applying this to C++. Instead, you should be using any form of RAII like smart pointers, for example.
Applying the Pattern
Consider the following excerpt from a code I just recently reviewed. I've simplified it for readability. This code has, deliberately, no error handling. This is what we are trying to solve.
static GstFlowReturn gst_example_extract_raw_buffer (GstNvMediaDemux * demux, GstBuffer * buffer, GstBuffer ** raw_buffer) { gint nvstatus = NVMEDIA_STATUS_OK; GstFlowReturn ret = GST_FLOW_OK; guint bytes_per_pixel = 2; NvMediaImageSurfaceMap surface_map = { 0 }; NvMediaImage *image = NULL; GstNvMediaMeta *meta = NULL; guint8 *data = NULL; g_return_val_if_fail (demux, GST_FLOW_ERROR); g_return_val_if_fail (buffer, GST_FLOW_ERROR); g_return_val_if_fail (raw_buffer, GST_FLOW_ERROR); *raw_buffer = NULL; meta = gst_buffer_get_nv_media_meta (buffer); image = meta->image_raw; nvstatus = NvMediaImageLock (image, NVMEDIA_IMAGE_ACCESS_READ, &surface_map); image_size = gst_example_compute_image_size (&surface_map); data = (guint8 *) g_malloc (image_size * sizeof (guint8)); nvstatus = NvMediaImageGetBits (image, data, image_size); *raw_buffer = gst_buffer_new_wrapped (buff, image_size); gst_example_analyze (*raw_buffer); NvMediaImageUnlock (meta->image_raw); return ret; }
Error Handling
This code has to error handling at all. Let's add some, shall we? One first approach can be thought of as the following:
static GstFlowReturn gst_example_extract_raw_buffer (GstNvMediaDemux * demux, GstBuffer * buffer, GstBuffer ** raw_buffer) { /* Declarations and pointer guards */ meta = gst_buffer_get_nv_media_meta (buffer); if (NULL == meta) { GST_ERROR_OBJECT (demux, "No NvMedia meta found, is the and NvMedia buffer?"); return GST_FLOW_ERROR; } image = meta->image_raw; nvstatus = NvMediaImageLock (image, NVMEDIA_IMAGE_ACCESS_READ, &surface_map); if (NVMEDIA_STATUS_OK != nvstatus) { GST_ERROR_OBJECT (demux, "Unable to lock NvMedia image: %d", nvstatus); return GST_FLOW_ERROR; } image_size = gst_example_compute_image_size (&surface_map); data = (guint8 *) g_malloc (image_size * sizeof (guint8)); if (NULL == data) { GST_ERROR_OBJECT (demux, "Unable to alloc buffer, system ran out of memory"); return GST_FLOW_ERROR; } nvstatus = NvMediaImageGetBits (image, data, image_size); if (NVMEDIA_STATUS_OK != nvstatus) { GST_ERROR_OBJECT (demux, "Unable to lock NvMedia image: %d", nvstatus); return GST_FLOW_ERROR; } *raw_buffer = gst_buffer_new_wrapped (buff, image_size); if (NULL == *raw_buffer) { GST_ERROR_OBJECT (demux, "Unable to wrap data, system ran out of memory"); return GST_FLOW_ERROR; } if (FALSE == gst_example_analyze (*raw_buffer)) { GST_ERROR_OBJECT (demux, "Some business logic error"); return GST_FLOW_ERROR; } NvMediaImageUnlock (meta->image_raw); return ret; }
This is the classical single input multiple output' scenarios. Hope you noticed the horrors in this approach :)[1]
Freeing Resources
Let's deal with the forgotten resources.
static GstFlowReturn gst_example_extract_raw_buffer (GstNvMediaDemux * demux, GstBuffer * buffer, GstBuffer ** raw_buffer) { /* Declarations and pointer guards */ meta = gst_buffer_get_nv_media_meta (buffer); if (NULL == meta) { GST_ERROR_OBJECT (demux, "No NvMedia meta found, is the and NvMedia buffer?"); return GST_FLOW_ERROR; } image = meta->image_raw; nvstatus = NvMediaImageLock (image, NVMEDIA_IMAGE_ACCESS_READ, &surface_map); if (NVMEDIA_STATUS_OK != nvstatus) { GST_ERROR_OBJECT (demux, "Unable to lock NvMedia image: %d", nvstatus); return GST_FLOW_ERROR; } image_size = gst_example_compute_image_size (&surface_map); data = (guint8 *) g_malloc (image_size * sizeof (guint8)); if (NULL == data) { GST_ERROR_OBJECT (demux, "Unable to alloc buffer, system ran out of memory"); NvMediaImageUnlock (meta->image_raw); /* <---- Freeing a resource */ return GST_FLOW_ERROR; } nvstatus = NvMediaImageGetBits (image, data, image_size); if (NVMEDIA_STATUS_OK != nvstatus) { GST_ERROR_OBJECT (demux, "Unable to lock NvMedia image: %d", nvstatus); NvMediaImageUnlock (meta->image_raw); /* <---- Freeing a resource */ g_free (data); /* <--------------------------- Freeing a resource */ return GST_FLOW_ERROR; } *raw_buffer = gst_buffer_new_wrapped (buff, image_size); if (NULL == *raw_buffer) { GST_ERROR_OBJECT (demux, "Unable to wrap data, system ran out of memory"); NvMediaImageUnlock (meta->image_raw); /* <---- Freeing a resource */ g_free (data); /* <--------------------------- Freeing a resource */ return GST_FLOW_ERROR; } if (FALSE == gst_example_analyze (*raw_buffer)) { GST_ERROR_OBJECT (demux, "Some business logic error"); NvMediaImageUnlock (meta->image_raw); /* <---- Freeing a resource */ gst_buffer_unref (*raw_buf); /* <------------- Freeing a resource */ return GST_FLOW_ERROR; } NvMediaImageUnlock (meta->image_raw); return ret; }
Do you see how the error handling starts to pile up with duplicate code?
Refactoring to SISO
First, let's analyze the resources acquisition and their order:
- ↑ No resources are freed here, leading to leaks and potential deadlocks!