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

[Mohammad-Reza Nabipoor]

Hi, Jose.

On Thu, Nov 12, 2020 at 09:34:20AM +0100, Jose E. Marchesi wrote:
> 
> 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()"


> > +/* 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`.