returning an error code from a function that has a pointer return type

I have tried several google searches, but probably use the wrong keywords to find this.

I have a function that returns a pointer type, now if the function detects an error condition I would like the caller to be able to check the returned type for a specific error condition instead of only being able to return NULL as a failure indication.

I understand that pointers are numeric values int/long etc. but what's the best approach to do this and make sure I don't define a possible proper pointer value as an error code?

example of what I would like to achieve (not actual code used);

Code:

#define BADPARAMETER -1

void* myfunc(int myparam)
{
    if (myparam < 0) return BADPARAMETER;

    ....

}

caller code

Code:

result = myfunc(somevalue);
if (result == BADPARAMETER)
{
   .... handle returned error
}

Probably has been done before, so any pointers to a solution are appreciated.

Thijs

One approach:

Code:

void* myfunc(int myparam, int* err_code)
{
    if (myparam < 0)
    {
        if (err_code)
        {
            *err_code = BADPARAMETER;
        }
        return NULL;
    }

    /* .... */
}

So, if the caller wants more information, the caller provides the address of an int to which the error code is saved. Otherwise, the only indication of an error is the null pointer that is returned.

That's usually the best way.

The other method you often see is for the code to have a global error state stored somewhere. When the function errors, it returns NULL and changes the error state to some error code. This can then be queried (if the caller is interested in WHY the function failed) either by reading the global directly, or using an "accessor" function (e.g. GetLastError()) and return the last error code seen. Lots of large frameworks use something similar (which query functions called GetError or whatever).

The only problem with that is that you have to set the error code to something in every function that *could* return an error using it, even if that's to some "NO_ERROR" constant, so that ignored errors don't propagate for the entire life of the program. Oh, and the calling functions still have to check for "NULL" returns and do something about the error in their return-checking code.

The problem I find with the method described by laserlight is that often people who call your function "forget" to pass you something useful, and what if they pass you a NULL pointer to store the error in? Then the function has to check for NULL pointers and ignore errors as appropriate, so you end up with a lot of code "inside" the functions. Whereas with the "GetError" method, the function just sets a global int to an error value and returns NULL and it's up to the caller to do all the leg-work (if they care about the error at all).

The only problem with that is that you have to set the error code to something in every function that *could* return an error using it, even if that's to some "NO_ERROR" constant, so that ignored errors don't propagate for the entire life of the program.

A lot of libraries don't go that route. Vendors will instead provide an interface to set the error code to a known "no error" value allowing client code to set the error code to that value if they care about handling a given error in advance.

Then the function has to check for NULL pointers and ignore errors as appropriate, so you end up with a lot of code "inside" the functions.

I'm not one for accepting that "precondition" and "postcondition" checks should be a thing; I also don't think that is a valid argument against error code parameters.

Code:

unsigned int gErrorCode;

unsigned int GetErrorCode();

void SetErrorCode
(
    unsigned int fErrorCode
);

void * CraftSomething
(
    int fMyParameter
)
{
    if(fMyParameter < 0)
    {
        SetErrorCode(INVALIDPARAMETER);
        return(NULL);
    }
    else
    {
        // ...
    }
}

Code:

void * CraftSomething
(
    int fMyParameter
  , unsigned int * fErrorCode
)
{
    unsigned int lErrorCode = 0;
    if(!fErrorCode)
    {
        fErrorCode = &lErrorCode;
    }
    if(fMyParameter < 0)
    {
        fErrorCode = INVALIDPARAMETER;
        return(NULL);
    }
    else
    {
        // ...
    }
}

The implementations of real functionality isn't that different.

[Edit]
You should make a far reaching design decision like this based on how it affects client code.

Yea, I'm not sure how that "not" got there...
[/Edit]

Soma

Thanks for the quick and thorough responses. So my googling didn't turn anything up because it can't be done that way.
Disappointing, but not giving up, I created the test code below;

Code:

static char _____Errors[2];
void* MYLIB_NOMEMORY = &_____Errors[0];
void* MYLIB_BADPARAMETER = &_____Errors[1];
void* MYLIB_SOMEERROR = &_____Errors[2];


char* myfunc(int someparam)  // use char to test compiler type checking
{
    if (someparam < 0) return (char *)MYLIB_BADPARAMETER;
    if (someparam == 0) return (char *)MYLIB_SOMEERROR;
    return NULL;
}


void checkvalue(int param)
{
    void* ptr = myfunc(param);
    if (ptr == MYLIB_BADPARAMETER)
    {
        printf("Bad parameter\n");
    }
    else
    {
        if (ptr == MYLIB_SOMEERROR)
        {
            printf("Some error\n");
        }
        else
        {
            printf("nice, finally a proper value!\n");
        }
    }
}


int _tmain(int argc, _TCHAR* argv[])
{
    checkvalue(-2);
    checkvalue(-1);
    checkvalue(0);
    checkvalue(1);
    checkvalue(2);
    return 0;
}

It works, just as the regular macros work except that it uses variables in this case. Writing and reading source code are just as usual, only caveat is the explicit cast to the char* return type of the errors, but that will be verified at compile time.

So if I (c newbie) can come up with this, and you didn't (c experts), than I am probably missing some serious issues with this one.

So whats wrong with it?

test code

I hate you and hope you suffer a minor torment... like a bad hangnail. I hope you get a bad hangnail.

So whats wrong with it?

O_o

Let's ignore all the important stuff and focus on the trivial which I can't help but think will speak to a newbie more than the important stuff possibly could.

So, didn't you notice that you wrote a function that exists only to call your function which does not actually report an error to the level at which that function was called?

You didn't write the `checkvalue' for the testbed so much as you wrote it to reduce complexity of calling the real `myfunc' function. (We all know you did so can we please skip the "knee jerk" reaction where you say you didn't?) That, if nothing else, is a huge issue.

Instead of actually calling `myfunc' and checking one result that says "Successful!" you have devised a way that requires client code to check every possible error condition. That is nothing short of insane, but I do admit, it is one hell of an accomplishment to shock me.

To be perfectly clear, you have wrapped `myfunc' with `checkvalue' precisely because you have broken the clear and obvious logic of returning an error code. Here you are only checking for two conditions because you know in advance that the target function only supports two error conditions. In the real world, you would have to check every possible condition at every invocation only you can't because new conditions can be added at any time. Even if you devise a way to do this "magically" (I also hate anyone who helps you in that project.) you still have to conceptually check every case every invocation which will drag performance down at an alarming rate.

In other words, the "nice, finally a proper value!\n" isn't valid. You've only eliminated two of infinitely many (well 2^32-1 anyway) cases. You can't know at this point in time that you will not add another error case to `myfunc' later.

The thing is, you actually already taken the time to wrap the actual function; you've already realized that you will never issue those checks every time you want to invoke the function. You've already found and promptly eliminated this insanity by wrapping it up so you don't have to deal with it.

Actually, I can't believe how broke this is; the longer I think about it the more ways I think of to break it.

I hate you!

Soma

Ick.

It looks ugly, it introduces unnecessary globals, and you have to do handfuls of comparisons for every single function return. That's fine for a tiny program, but as you scale up that will become a huge performance drag (and sod having to code the checkvalue routine and maintain it every time there's a new type of error you want to catch).

You could have just let checkvalue do the same as laserlight suggested - little to no performance hit, easier syntax, much more maintainable and much more expandable when your code changes. Maybe for your case it "works" but it's hardly pretty and now do a good programming exercise: Imagine that someone else has to come along and use your "myfunc" function without your assistance from *their* code. They now would have to duplicate ALL your error handling code and your horrible static char array in order to use the damn thing.

This makes me physically ill.

I feel like I'm watching a horror movie where the bad guy stops in the middle of slaughtering an innocent turning to face the camera and actually saying "I'm Bob and I'll be your bad guy for this movie.". It isn't funny. It isn't clever. It is sick, sad, and pathetic!

Soma

Isn't using errno a simpler solution ?
It is global state but I don't think that is much of a problem in a procedural language, when guaranteed to be thread safe by the standards.

Originally Posted by manasij7479

Isn't using errno a simpler solution ?
It is global state but I don't think that is much of a problem in a procedural language, when guaranteed to be thread safe by the standards.

There is nothing in the standards that guarantees usage of errno to be "thread safe".

Generally, when using functions that set errno, it is necessary to either (1) ensure they will never touch errno (i.e. code to avoid error states that needs to be tested for) or (2) synchronise access to the functions, so that access of errno is synchronised when access is attempted from multiple threads.

It is a basic fact of multithreaded development that no object (global or otherwise) can protect itself from concurrent access. It is always up to code which uses that object (eg the callers of functions that modify errno) to handle synchronisation, not the objects themselves.

O_o

The standard I know (C99) doesn't guarantee thread safety, but I do think it references atomicity in the latest standard (C11). That though only guarantees atomic reads and writes meaning that the issues grumpy references still come into play.

That said, some implementations bypass the concurrency issue by using thread local storage (using compiler intrinsic). You can't rely on this.

Soma

Originally Posted by grumpy

It is a basic fact of multithreaded development that no object (global or otherwise) can protect itself from concurrent access.

But errno may not be an object...but just an obfuscation.. afaik. (It can link to a function call like mechanism, which implements the locking code, unless it is local to threads)
My man page says:

errno is defined by the ISO C standard to be a modifiable lvalue of type int, and must not be explicitly
declared; errno may be a macro. errno is thread-local; setting it in one thread does not affect its value in
any other thread.

Originally Posted by phantomotap

That said, some implementations bypass the concurrency issue by using thread local storage (using compiler intrinsic). You can't rely on this.

What is the difference ?
I thought they were exactly the same as far as the code is concerned.

It doesn't matter what `errno' really is if it is shared between threads because of how it is used.

I'll leave these examples here for you to examine.

Code:

// This is not sufficient to protect `errno' use
// patterns because atomicity only guarantees
// reads and writes will be complete within the
// context of one thread of execution.
__attribute__((__atomic__)) int errno;

bool DoSomething1()
{
    // ...
    if(/* ??? */)
    {
        // This is guaranteed to set `errno' to the value 1
        // completely and truly even if another thread
        // tries to set a different value at the same time.
        errno = 1;
        return(false);
    }
    // ...
}

void DoSomething2()
{
    // ...
    errno = 0;
    if(!DoSomething1())
    {
        // The `errno' is only atomic with respect to reads and
        // writes. The value `errno' has is going to be the
        // complete and true value any thread set.
        // However, it is not guaranteed to be the value
        // `DoSomething1' set because another thread may 
        // have changed the value after `DoSomething1' set the
        // value but before we were able to examine it here.
        printf("%d", errno);
    }
    // ...
}

Code:

__attribute__((__tls__)) int errno;

bool DoSomething1()
{
    // ...
    if(/* ??? */)
    {
        errno = 1;
        return(false);
    }
    // ...
}

void DoSomething2()
{
    // ...
    errno = 0;
    if(!DoSomething1())
    {
        // Because we have implemented `errno' in terms of
        // thread local storage no other threads can
        // have modified `errno'. We have the exact `errno'
        // value `DoSomething1' set.
        printf("%d", errno);
    }
    // ...
}

Soma

So... I crafted evil code and will burn in hell forever...

Originally Posted by phantomotap

Instead of actually calling `myfunc' and checking one result that says "Successful!" you have devised a way that requires client code to check every possible error condition. That is nothing short of insane, but I do admit, it is one hell of an accomplishment to shock me.

So here's my big miss. Thx for pointing that out, that is indeed a big issue.

Originally Posted by phantomotap

In the real world, you would have to check every possible condition at every invocation only you can't because new conditions can be added at any time. Even if you devise a way to do this "magically" (I also hate anyone who helps you in that project.) you still have to conceptually check every case every invocation which will drag performance down at an alarming rate.

In other words, the "nice, finally a proper value!\n" isn't valid. You've only eliminated two of infinitely many (well 2^32-1 anyway) cases. You can't know at this point in time that you will not add another error case to `myfunc' later.

Those arguments can be overcome, the array has consecutive elements, hence pointer arithmetic can be used ;

Code:

#define MYLIB_MAXERROR 30
static char _____Errors[MYLIB_MAXERROR];
void* MYLIB_NOMEMORY = &_____Errors[0];
void* MYLIB_BADPARAMETER = &_____Errors[1];
void* MYLIB_SOMEERROR = &_____Errors[2];
#define MYLIB_ISERROR(ptr) (ptr >= &_____Errors[0] && ptr <= &_____Errors[MYLIB_MAXERROR])

with usage;

Code:

void dosomethingthatmightcauseanerror(int param)
{
    void* ptr = myfunc(param);
    if (MYLIB_ISERROR(ptr))
    {
        printf("We had an error\n");
    }
    else
    {
        printf("A decent value\n");
    }
}

Originally Posted by ledow

It looks ugly

Yep, and I just made it even worse

Originally Posted by ledow

it introduces unnecessary globals

Is that such a bad thing, compared to lists of integer macros to define errors, which seems to be quite common in c? Seems the same to me.


But as said, I think laserlights suggestion would be the cleanest solution. I think I'll go for that.

Thanks for your time folks!