[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failur
From: |
Alon Levy |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure |
Date: |
Mon, 26 Mar 2012 20:51:25 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
>
> 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().
>From a brief due dilligence (git grep -A10 fopen | grep EINTR | wc -l ==
0) I gather that no one ever checks EINTR after fopen, so I'm going to
avoid the complexity of possibly blocking in qemu_fopen_err and just
note in the qapi-schema wherever an qemu_fopen_err derived error might
be returned that the generic OpenFileFailed could indicate an EINTR.
I think it will at least be easier to fix it later in a single location,
that is at qemu_fopen_err, in a later patch.
>
> > + {
> > + .error_fmt = QERR_EACCES,
> > + .desc = "Cannot access file'",
> > + },
>
> We already have QERR_PERMISSION_DENIED.
>
> > + {
> > + .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.
>
> > + {
> > + .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.
>
> > +
> > +#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': {} }"
> >
>
>
[Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure, Alon Levy, 2012/03/18
Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure,
Alon Levy <=
Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure, Alon Levy, 2012/03/26
[Qemu-devel] [PATCH v3 3/5] vga_hw_screen_dump: add Error** param, Alon Levy, 2012/03/18
[Qemu-devel] [PATCH v3 4/5] qapi: convert screendump, Alon Levy, 2012/03/18