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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 23 Mar 2012 21:53:03 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Fri, Mar 23, 2012 at 11:21:25AM -0300, Luiz Capitulino wrote:
> > On Sun, 18 Mar 2012 19:29:09 +0100
> > Alon Levy <alevy@redhat.com> 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 <alevy@redhat.com>
> > > ---
> > >  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? 

I had forgotten about our discussion about this. You could add InvalidAccess,
although I think we already use QERR_PERMISSION_DENIED as EACCESS...

> 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.

You can trim it. The important point is to avoid creating new descriptions.

> 
> > 
> > > +    {
> > > +        .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).

No, if the command has a good description, like:

 @AlreadyExists, if the file already exists

Then it's possible to know what happened and what the client should do.

> 
> > 
> > > +
> > > +#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': {} }"
> > >  
> > 
> > 
>