Why leave the error handling to the caller?

[Richard Heathfield]

CBFalconer said:

And what do you object to?

The exit() call.

 

[Richard Tobin]

Richard Heathfield wrote:

And what do you object to?

Exiting without an error message is very unhelpful. If there's no
practical way to recover from the error, you should at least say what
the error is.

I'd also recommend saying how many bytes you were trying to allocate:
I've more than once seen messages along the lines of "failed to
allocate 4294967295 bytes" which immediately pointed to bugs in the
code.

-- Richard

 

[Richard Heathfield]

Dave Vandervies said:

Richard Heathfield wrote:


From the OpenSSH 4.6 source code:
--------
void *
xmalloc(size_t size)
{
void *ptr;

if (size == 0)
fatal("xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
fatal("xmalloc: out of memory (allocating %lu bytes)",
(u_long) size);
return ptr;
}

Send them all to Marketing!

I wouldn't exactly call SSH "not real world". I think you'd have
trouble arguing that the people who wrote this code didn't carefully
consider what was and wasn't appropriate behavior on memory allocation
failure before they wrote it this way, even if you disagree with their
conclusion.

Well, since I wasn't in on their discussion I obviously can't argue that
they did, or that they did not, carefully consider any of the above.
What I can argue is that a library function has no business taking the
decision of whether to continue or to terminate out of the hands of the
application programmer. Since xmalloc does take that decision out of
the hands of the programmer, I wouldn't want it in any program I ever
wrote. Student programming, through and through.

 

[Richard Tobin]

What I can argue is that a library function has no business taking the
decision of whether to continue or to terminate out of the hands of the
application programmer. Since xmalloc does take that decision out of
the hands of the programmer, I wouldn't want it in any program I ever
wrote.

Provided it documents what it does, you have the decision of whether
to call it or not. Frequently the purpose of a function called
"xmalloc" is to provide the application programmer with a quick way to
"malloc() or quit".

Or perhaps you're arguing that the decision has been taken out of the
hands of programmers who call functions that call xmalloc(), assuming
that those functions are intended to be used as a library. Ideally
they would return an error status, but often it would be a great
overhead to be able to clean up enough to continue after a malloc()
failure. Whether it's worth the effort is just one of those tradeoffs
you make all the time - do we expect users to need to recover? will
enough of them need it to justify the work? will writing the program
become so boring that I won't bother writing it if I have to do that?

-- Richard

 

[Richard Heathfield]

Richard Tobin said:

Or perhaps you're arguing that the decision has been taken out of the
hands of programmers who call functions that call xmalloc(), assuming
that those functions are intended to be used as a library.
Yes.

Ideally
they would return an error status, but often it would be a great
overhead to be able to clean up enough to continue after a malloc()
failure. Whether it's worth the effort is just one of those tradeoffs
you make all the time - do we expect users to need to recover? will
enough of them need it to justify the work? will writing the program
become so boring that I won't bother writing it if I have to do that?

Suits me. I'll write it instead, once the cheque clears.

 

[Eric Sosman]

Richard Tobin wrote On 06/20/07 12:54,:

Provided it documents what it does, you have the decision of whether
to call it or not. Frequently the purpose of a function called
"xmalloc" is to provide the application programmer with a quick way to
"malloc() or quit".

Or perhaps you're arguing that the decision has been taken out of the
hands of programmers who call functions that call xmalloc(), assuming
that those functions are intended to be used as a library. Ideally
they would return an error status, but often it would be a great
overhead to be able to clean up enough to continue after a malloc()
failure. Whether it's worth the effort is just one of those tradeoffs
you make all the time - do we expect users to need to recover? will
enough of them need it to justify the work? will writing the program
become so boring that I won't bother writing it if I have to do that?

These are questions that can only be answered in the
context of a particular program, not in that of a general-
purpose library. I myself quite often use a malloc-or-quit
wrapper, but I only call it from program-specific functions
and never from library routines written for reusability.
If my skip list can't allocate memory it returns a failure
code; it doesn't yank the rug out from under the rest of
the program. On the other hand, my "initialize the data
structures that are specific to this program, without which
the program has no hope of running" function quite happily
calls malloc-or-die. Or fopen-or-die, or even parse-input-
line-or-die-on-syntax-error.

Program-level decisions are feasible because their scope
is known: You know what the program's purposes and context
are. Library-level decisions are made *without* knowledge
of scope, and hence should follow the "first, do no harm"
rule.

 

[Army1987]

And what do you object to? Kindly elide 'missing braces' and the
possibility of sz == 0.

Quitting the program without giving anybody a clue of why?

 

[Army1987]

to call it or not. Frequently the purpose of a function called
"xmalloc" is to provide the application programmer with a quick way to
"malloc() or quit".

#include <iso646.h>
#include <stdlib.h>
#define quit exit(EXIT_FAILURE)
/*

....whoops, both operands of || must have scalar type...
*/
#undef quit
#define quit (exit(EXIT_FAILURE), 0)

....
(p = malloc(200)) or quit; /* ;-) */

 

[Christopher Benson-Manica]

Malcolm McLean said:

No-one wants to play games written by incompetent programmers, either.

Quite so, but the fact that no one wants to run an OS written by
incompetent programmers unfortunately does not remove the necessity of
doing so. If the choice is between releasing a game with bugs (which
may or may not be fixed) and not releasing the game, the choice for
most game developers is clear.

 

[Richard Heathfield]

Christopher Benson-Manica said:

Quite so, but the fact that no one wants to run an OS written by
incompetent programmers unfortunately does not remove the necessity of
doing so. If the choice is between releasing a game with bugs (which
may or may not be fixed) and not releasing the game, the choice for
most game developers is clear.

Oh, the choice they /do/ make is clear, yes. And the choice they
/should/ make is clear, too. And they are not the same.

 

[Christopher Benson-Manica]

Richard Tobin said:

Yes.

I'm still not sure I see the problem. Functions that call xmalloc()
ought to be documented to have the die-on-malloc()-failure behavior -
if you don't trust a library's documentation, you're already in the
wilderness - and given such documentation, programmers using the
library still have the option not to use such functions or (more
likely) not to use the library at all.

I do agree with what I believe to be your real point, namely, that
essentially no programmers writing real programs will be willing to
invoke functions with the possibility of bringing the entire program
down.

 

[Richard Tobin]

Ideally
they would return an error status, but often it would be a great
overhead to be able to clean up enough to continue after a malloc()
failure. Whether it's worth the effort is just one of those tradeoffs
you make all the time - do we expect users to need to recover? will
enough of them need it to justify the work? will writing the program
become so boring that I won't bother writing it if I have to do that?

[/QUOTE]

Suits me. I'll write it instead, once the cheque clears.

Well, there you put your finger on it. There is a cost to writing more
flexible code, whether in time or money. Sometimes it's worth it, and
sometimes it isn't.

-- Richard

 

[Richard Tobin]

Program-level decisions are feasible because their scope
is known: You know what the program's purposes and context
are. Library-level decisions are made *without* knowledge
of scope, and hence should follow the "first, do no harm"
rule.

I take your point, but disagree with your conclusion. If you don't
know how a library will be used, it might be a waste of effort to take
account of all possibilities of reuse. The cost of allowing for them
might be unjustified given their probability. You just have to judge
each case on its merits.

-- Richard

 

[Keith Thompson]

From the OpenSSH 4.6 source code:
--------
void *
xmalloc(size_t size)
{
void *ptr;

if (size == 0)
fatal("xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
fatal("xmalloc: out of memory (allocating %lu bytes)", (u_long) size);
return ptr;
}
--------
fatal() writes an error message and terminates; this is the only
place in any of the OpenSSH tools (except ssh-keyscan, which isn't
typically used other than for configuration purposes) where malloc
is called directly, and everything else goes through here. Similar
wrappers exist for every other function that dynamically allocates
memory.

I wouldn't exactly call SSH "not real world". I think you'd have
trouble arguing that the people who wrote this code didn't carefully
consider what was and wasn't appropriate behavior on memory
allocation failure before they wrote it this way, even if you
disagree with their conclusion.

In this case, xmalloc() is not part of some general-purpose library,
it's part of a particular application (or set of applications); the
authors of xmalloc() and the authors of the code that uses it are
likely the same people.

In the context of a text editor, using something like xmalloc() would
be a very bad idea; if I run out of memory trying to insert some huge
thing into my buffer, then just *that operation* should fail, and I
should be able to continue editing, or at least to save the changes
I've made so far.

If I'm using an ssh client, there is no such context that needs to be
saved. If the client program, or even the server program, runs out of
memory, there is likely no reasonable way to recover other than
terminating the program.

 

[CBFalconer]

Exiting without an error message is very unhelpful. If there's no
practical way to recover from the error, you should at least say what
the error is.

I'd also recommend saying how many bytes you were trying to allocate:
I've more than once seen messages along the lines of "failed to
allocate 4294967295 bytes" which immediately pointed to bugs in the
code.

A point. A veritable point.

 

[CBFalconer]

.... snip ...

Well, since I wasn't in on their discussion I obviously can't argue
that they did, or that they did not, carefully consider any of the
above. What I can argue is that a library function has no business
taking the decision of whether to continue or to terminate out of
the hands of the application programmer. Since xmalloc does take
that decision out of the hands of the programmer, I wouldn't want
it in any program I ever wrote. Student programming, through and
through.

Nonsense. It is entirely up to the programmer whether to call
xmalloc or malloc.

 

[Malcolm McLean]

Flash Gordon said:



I took a brief look, but I felt that *either* Malcolm was trying to
write bad code (in which case he was being disingenuous at best) or he
was trying to write good code (in which case I pity his customers).

Hanlon's Razor suggests the latter.

There is no pleasing some people.
You wanted it to compile, so now it compiles. But now that's not good
enough. I've even taken out the str identifiers in case those were the cause
of the complaint.
It was formatted in Microsoft's free compiler, and I cut and pasted. Somehow
the indents must have gone wrong, which I didn't notice and apologise for.

 

[Malcolm McLean]

Nonsense. It is entirely up to the programmer whether to call
xmalloc or malloc.

The choice depends what your program is doing.
Mostly, if the OS will not give you a hundred bytes of memory, then your
program's functioning is likely to be the least of the user's worries, and
the best thing to do is terminate.
However if you hold precious data, but not too precious, then you might want
to allow the user to terminate other programs, so you can continue. If the
data is too precious, then of course this is irrelevant. You need to be sure
the data can be recovered, even if someone flicks the off switch.

The problem is that if you are writing a subroutine rather than a program,
you don't know what the program that calls it will be used for. So xmalloc()
is useless, unless we have a convention that all subroutines will call
exit() or the failure of small allocations, in which case it is up to caller
to call atexit() and recover as he may - which means a different style of
programming because you might have partially constructed objects in memory
which you have to save on demand - and without making any further memory
allocation calls.

 

[Malcolm McLean]

Christopher Benson-Manica said:


Oh, the choice they /do/ make is clear, yes. And the choice they
/should/ make is clear, too. And they are not the same.

If we were releasing a medical app which was not debugged and in consequence
killed one person a year, the moral choice would be clear.

However we are releasing a game which spoils some kid's fun once a year -
remembering that for every time he can't kill the evil wizard because it
crashes when you cast magic mirror and invisibility at the same time, he
can't kill the evil wizard because mum has called him for tea.
Then we've sunk millions of pounds of development money into this thing, and
every day the game is not released is costing several hundred pounds in bank
interest charges alone. We've not selling to the NHS (British government
health service which is spending 12 billion pounds on a password-protected
Wiki to hold patient records) here. If we don't release when the marketing
people want, it could easily take down the company.

 

[Richard Bos]

From the OpenSSH 4.6 source code:
--------
void *
xmalloc(size_t size)
{
void *ptr;

if (size == 0)
fatal("xmalloc: zero size");
ptr = malloc(size);
if (ptr == NULL)
fatal("xmalloc: out of memory (allocating %lu bytes)", (u_long) size);
return ptr;
}

I wouldn't exactly call SSH "not real world". I think you'd have trouble
arguing that the people who wrote this code didn't carefully consider
what was and wasn't appropriate behavior on memory allocation failure
before they wrote it this way, even if you disagree with their conclusion.

That's as may be, but I do hope that that code didn't find its way into
the SHTTP parts of, e.g., Firefox. Crash&dump is all very well for a
small command line tool, but if an application can hold any serious
amount of data in memory, it's an evil thing to do to your users.

Richard