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

[Luiz Capitulino]

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

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