Re: [PATCH v2] Make error handling of public API consistent

[Jose E. Marchesi]

>> >  pk_ios
>> >  pk_ios_cur (pk_compiler pkc)
>> >  {
>> > +  pkc->status = PK_OK;
>> >    return (pk_ios) ios_cur ();
>> >  }
>> 
>> Can't ios_cur fail?
>> 
>> >  pk_ios
>> >  pk_ios_search (pk_compiler pkc, const char *handler)
>> >  {
>> > +  pkc->status = PK_OK;
>> >    /* XXX use pkc */
>> >    return (pk_ios) ios_search (handler);
>> >  }
>> 
>> Ditto for ios_search.  Can't it fail?  What if an IOS having hte given
>> handler cannot be found?
>> 
>> > @@ -413,6 +433,7 @@ pk_ios_search (pk_compiler pkc, const char *handler)
>> >  pk_ios
>> >  pk_ios_search_by_id (pk_compiler pkc, int id)
>> >  {
>> > +  pkc->status = PK_OK;
>> >    /* XXX use pkc */
>> >    return (pk_ios) ios_search_by_id (id);
>> >  }
>> 
>> Likewise.
>> 
>
>
> All `ios_cur`, `ios_search` and `ios_search_by_id` functions will return 
> `NULL`
> when the entry does not exist.
> I don't think that's a failure or error. Everything in the library worked
> as expected and not found the requested item.

Oh, of course.
We have to return PK_IOS_INVALID in that case, and set status to PK_OK.

>> > -  return ios_open (handler, flags, set_cur_p);
>> > +  enum
>> > +  {
>> > +    ELEN = 8,
>> > +  };
>> 
>> You don't need an enumerated for that.
>> 
>
> In general I use `enum` to define constant integers (instead of using
> `const int`). Because they have less limitations than `const int`s (e.g., I
> can use them in cases of a `switch` statement.
> Just a personal preference :)
>
>
>> Actually, I would use a switch statement instead of a table: the later
>> is too elaborated IMO.  Also, we cannot assert in library code: in case
>> ios_open returns an error you don't know (i.e. the `default' in the
>> switch) just return PK_ERROR.
>> 
>
> I chose the table over the `switch` statement because
>   1) they are shorter
>   2) I thought tables are faster (which turned out to be wrong; compilers
>      generate exactly the same code (at least `gcc` and `clang`)).
> I'll use a `switch` statement.

Ok, thanks :)

> I put an `assert` to increase the chance of finding the missing error code in
> tests. Otherwise how can we spot an inconsistency in error codes?
> It's an internal thing and we should cover all the errors; I even think
> we should put an `assert` in `default` case of `switch`, too. 

I wouldn't use asserts, really.  We can test inconsistency in error
codes indirectly by testing the proper behavior of the libpoke
interface.

> What about defining all error codes in a common header file?
> And use macros (or inline functions) to translate them among modules.
>
> e.g.,
>
> ```c
> #include "libpoke-errors.h"
> /* Which contains the following lines (and will be distribute along the 
> libpoke.h).
>   #define PK_OK 0
>   #define PK_ERROR 1
>   #define PK_ENOMEM 2
>   #define PK_EEOF 3
>   #define PK_EINVAL 4
> */
>
> #define E_IOS_OK      0  /* The operation was performed to completion,
>                           in the expected way.  */
> #define E_IOS_ERROR  -1  /* An unspecified error condition happened.  */
> #define E_IOS_ENOMEM -4  /* Memory allocation failure.  */
> #define E_IOS_EOF    -5  /* End of file / input.  */
> #define E_IOS_EINVAL -6  /* Invalid argument.  */
> #define E_IOS_EOPEN  -7  /* IO space is already open.  */
>
> /* Convert errors from E_IOS_* to PK_* */
> #define ERROR_IOS(e)                     \
>   ({                                     \
>     int _e = PK_ERROR;                   \
>                                          \
>     switch (e)                           \
>       {                                  \
>       case E_IOS_OK: _e = PK_OK;         \
>       case E_IOS_ERROR: _e = PK_ERROR;   \
>       case E_IOS_ENOMEM: _e = PK_ENOMEM; \
>       case E_IOS_EOF: _e = PK_EEOF;      \
>       case E_IOS_EINVAL: _e = PK_EINVAL; \
>       case E_IOS_EOPEN: _e = PK_EINVAL;  \
>       default: assert (0);               \
>       }                                  \
>     _e;                                  \
>   })
> ```
>
> WDYT?

I don't like that approach.  Such a header file would a) need to be
distributed and b) contain logic that doesn't belong outside of libpoke,
such as the mapping from IOS error codes to PK error codes.

> And I think having a consistent error code everywhere is good thing
> (currently some internal functions return 1 as success and some others
> return 0 as success).  Using (e.g.,) "libpoke-error.h" we can use
> these errors across all modules.

Using consistent error codes is nice, but always respecting modularity.
Using the same codes (or even the same notion of what an error is and
what not) works against that modularity.