Re: [PATCH v3] libpoke,poke,testsuite: Make error handling of public API consistent

[Jose E. Marchesi]

Hi Mohammad.

>  pk_ios_open (pk_compiler pkc,
>               const char *handler, uint64_t flags, int set_cur_p)
>  {
> +  int ret;
> +
>    /* XXX use pkc */
> -  return ios_open (handler, flags, set_cur_p);
> +  if ((ret = ios_open (handler, flags, set_cur_p)) >= 0)
> +    return ret;
> +
> +  switch (ret)
> +    {
> +    case IOS_ERROR: pkc->status = PK_ERROR; break;
> +    case IOS_ENOMEM: pkc->status = PK_ENOMEM; break;
> +    case IOS_EOF: pkc->status = PK_EEOF; break;
> +    case IOS_EINVAL:
> +    case IOS_EOPEN:
> +      pkc->status = PK_EINVAL;
> +      break;
> +    default:
> +      pkc->status = PK_ERROR;
> +    }
> +  return PK_IOS_INVAL;
>  }

Shouldn't that be:

case IOS_ERROR: PK_RETURN (PK_ERROR); break;
case IOS_ENOMEM: PK_RETURN (PK_ENOMEM); break;
...

?

> +/* Error code of last error.

Of last operation.

> +
> +   If PKC is not NULL, return the status of the last API function invocation.
> +
> +   One of the following values:
> +     PK_OK, PK_ERROR, PK_ENOMEM, PK_EEOF, PK_EINVAL
> +   Otherwise, return PK_ERROR.  */

I would say something like "This function returns the status
corresponding to the given PK compiler.  This status is updated by the
last call performed in the API, and is one of the PK_* values defined
above."

There is no need to enumerate the possible values that are returned.
Just refer to the list of defines.

> @@ -268,9 +280,10 @@ uint64_t pk_ios_size (pk_ios ios) LIBPOKE_API;
>  uint64_t pk_ios_flags (pk_ios ios) LIBPOKE_API;
>  
>  /* Open an IO space using a handler and if set_cur is set to 1, make
> -   the newly opened IO space the current space.  Return PK_IOS_ERROR
> +   the newly opened IO space the current space.  Return PK_IOS_INVAL
>     if there is an error opening the space (such as an unrecognized
> -   handler), the ID of the new IOS otherwise.
> +   handler). Otherwise, the ID of the new IOS.
> +   The error code can be retrieved by PK_ERRNO function.

Please refer to pk_errno instead of PK_ERRNO in this and similar
contexts.

>
>     FLAGS is a bitmask.  The least significant 32 bits are reserved for
>     common flags (the PK_IOS_F_* above).  The most significant 32 bits
> @@ -279,8 +292,7 @@ uint64_t pk_ios_flags (pk_ios ios) LIBPOKE_API;
>     If no PK_IOS_F_READ or PK_IOS_F_WRITE flags are specified, then the
>     IOS will be opened in whatever mode makes more sense.  */
>  
> -#define PK_IOS_OK     0
> -#define PK_IOS_ERROR -1
> +#define PK_IOS_INVAL (-1)

Please add a comment documenting that PK_IOS_INVAL represents an invalid
IO space identifier.  I would really name it PK_IOS_NOID.