Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure

[Alon Levy]

On Fri, Mar 23, 2012 at 11:21:25AM -0300, Luiz Capitulino wrote:
> On Sun, 18 Mar 2012 19:29:09 +0100
> Alon Levy <address@hidden> wrote:
> 
> > Added:
> > 
> > QERR_EINTR
> > QERR_EACCES
> > QERR_EEXIST
> > QERR_OPEN_FILE_EMFILE
> > QERR_ENOSPC
> > QERR_EPERM
> > QERR_READ_ONLY
> > QERR_ENOTDIR
> > QERR_EFBIG
> > 
> > Signed-off-by: Alon Levy <address@hidden>
> > ---
> >  qerror.c |   36 ++++++++++++++++++++++++++++++++++++
> >  qerror.h |   27 +++++++++++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> > 
> > diff --git a/qerror.c b/qerror.c
> > index f55d435..4915939 100644
> > --- a/qerror.c
> > +++ b/qerror.c
> > @@ -213,6 +213,42 @@ static const QErrorStringTable qerror_table[] = {
> >          .desc      = "Could not open '%(filename)'",
> >      },
> >      {
> > +        .error_fmt = QERR_EINTR,
> > +        .desc      = "Interrupted open of '%(filename)'",
> > +    },
> 
> There are two problems here. First, the QERR_INTR macro doesn't have the
> filename argument, so this will crash.

Ugh, missed that.

> 
> The second problem is that, I've talked to Anthony and EINTR should not
> be returned to clients, it should be handled intead. So, please handle it
> during write().
> 
> > +    {
> > +        .error_fmt = QERR_EACCES,
> > +        .desc      = "Cannot access file'",
> > +    },
> 
> We already have QERR_PERMISSION_DENIED.
> 

So you want to have both error conditions (EACCESS and EPERM) translated
to the more generic PERMISSION_DENIED? This means that information is
lost when looking at the error value. The main thing would be to show
the filename, which we will do later as you said, so fine I guess.

> > +    {
> > +        .error_fmt = QERR_EEXIST,
> > +        .desc      = "File already exists'",
> > +    },
> 
> Can you use the description provided by man 3 errno? This is valid for all
> errors you're adding.

It's a bit lengthy. But sure.

> 
> > +    {
> > +        .error_fmt = QERR_OPEN_FILE_EMFILE,
> > +        .desc      = "Max open files when opening file'",
> > +    },
> > +    {
> > +        .error_fmt = QERR_ENOSPC,
> > +        .desc      = "No space left opening file'",
> > +    },
> > +    {
> > +        .error_fmt = QERR_EPERM,
> > +        .desc      = "Permission denied (EPERM) opening file'",
> > +    },
> > +    {
> > +        .error_fmt = QERR_READ_ONLY,
> > +        .desc      = "Read only filesystem opening file'",
> > +    },
> > +    {
> > +        .error_fmt = QERR_ENOTDIR,
> > +        .desc      = "Directory related error opening file'",
> > +    },
> > +    {
> > +        .error_fmt = QERR_EFBIG,
> > +        .desc      = "File too big opening'",
> > +    },
> > +    {
> >          .error_fmt = QERR_PERMISSION_DENIED,
> >          .desc      = "Insufficient permission to perform this operation",
> >      },
> > diff --git a/qerror.h b/qerror.h
> > index e26c635..ddc04e8 100644
> > --- a/qerror.h
> > +++ b/qerror.h
> > @@ -181,6 +181,33 @@ QError *qobject_to_qerror(const QObject *obj);
> >  #define QERR_OPEN_FILE_FAILED \
> >      "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
> >  
> > +#define QERR_OPEN_FILE_EMFILE \
> > +    "{ 'class': 'OpenFileEMFILE', 'data': {} }"
> > +
> > +#define QERR_EINTR \
> > +    "{ 'class': 'EINTR', 'data': {} }"
> > +
> > +#define QERR_EACCES \
> > +    "{ 'class': 'EACCES', 'data': {} }"
> > +
> > +#define QERR_EEXIST \
> > +    "{ 'class': 'EEXIST', 'data': {} }"
> 
> We use more descriptive names instead of using the errno name. Like 
> AlreadyExists.
> Please fix it for errors you adding.

Actually this was on purpose, to allow someone catching it to know it
originated in a failed systemcall. So your way the catcher would have to
go read the source (or hopefully I'll choose similar names).

> 
> > +
> > +#define QERR_ENOSPC \
> > +    "{ 'class': 'ENOSPC', 'data': {} }"
> > +
> > +#define QERR_EPERM \
> > +    "{ 'class': 'EPERM', 'data': {} }"
> > +
> > +#define QERR_READ_ONLY \
> > +    "{ 'class': 'ReadOnly', 'data': {} }"
> > +
> > +#define QERR_ENOTDIR \
> > +    "{ 'class': 'ENOTDIR', 'data': {} }"
> > +
> > +#define QERR_EFBIG \
> > +    "{ 'class': 'EFBIG', 'data': {} }"
> > +
> >  #define QERR_PERMISSION_DENIED \
> >      "{ 'class': 'PermissionDenied', 'data': {} }"
> >  
> 
>