Always check your return codes?

Is there a bug here?

int do_work(char *psz) {
some_type *p = NULL;
int t;
if ((t = foo(&p, psz)) != 0) return t;
if ((t = bar(p)) != 0) return t;
if ((t = baz(p)) != 0) return t;
return 0; // success
}

Let's be clear that this is ambiguous.  We don't know what foo, bar and baz do and while it might be obvious from the calling patterns that maybe there's some pattern of resoure management going on, it's just not clear.  I don't want to spend a lot of time on good naming of functions, I want to get into resource and invariant management issues.

If I just change the names, it's pretty clear that there is a bug here:

int do_work(char *psz) {
resource *pr = NULL;
int t;
if ((t = open_file_as_resource(&pr, psz)) != 0) return t;
if ((t = swap_order_of_bytes_in_resource(pr)) != 0) return t;
if ((t = close_resource(pr)) != 0) return t;
return 0; // success
}

The bug of course is that if swap_order_of_bytes_in_resource() fails, we forget to close the resource.  The corrected version of this function apparently is:

int do_work(char *psz) {
resource *pr = NULL;
int t;
if ((t = open_file_as_resource(&pr, psz)) != 0) return t;
if ((t = swap_order_of_bytes_in_resource(pr)) != 0) { close_resource(pr); return t; }
if ((t = close_resource(pr)) != 0) return t;
return 0; // success
}

Or is it?  I just introduced an obvious bug in the source (it's not checking the return status of close_resource() in the error exit case).

I think the pattern is broken and a discussion of how the pattern is broken and what it takes to actually fix the pattern is what I'm going to write about for a while.  The fix for this particular instance of the pattern is probably well known but there are a lot of nuances and it's a gateway into larger patterns.

(note: I'm aware of how RAII helps here and I'm an advocate of it; I'm decomposing these code examples to the barest minimums so that we can see the defects present.  We'll see that in some cases the fixes for these kinds of problem exactly align with the code you would see generated for use of objects with deterministic lifetime management, be it C++ objects with destructors or CLR objects that implement IDisposable and are properly used in a using statement.)

Comments

• Anonymous
  April 26, 2005
  The comment has been removed
• Anonymous
  April 26, 2005
  Well I am headed somewhere with this. At first the problem looks amenable to simple solutions but I plan to develop motivation towards realizing that functions like close_resource() are actually very special and have to have very special contracts.
  
  I don't want to show my hand here, I'm trying to learn from RaymondC about delivering small digestable chunks of information.
• Anonymous
  May 29, 2009
  PingBack from http://paidsurveyshub.info/story.php?title=mgrier-s-weblog-always-check-your-return-codes