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.

Examples of different amounts of entry and output points.

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.

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 :)

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:

Analyzing the resource acquisition