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;
>> ...
>> 
>> ?
>>
>
>
> No. The function should return `PK_IOS_INVAL` to indicates that
>   "this is an invalid ios id; for more info check the pk_errno()"

Ok.

>> > +/* 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."
>
>
> Thanks. Writing a good comment is hard :)
>
>
>> 
>> 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.
>
>
> OK.
>
>
>> 
>> >
>> >     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.
>
> OK. I'll change it to `PK_IOS_NOID`.

Thanks.