On Thu, 15 Mar 2012 00:55:10 +0200
Alon Levy <al...@redhat.com> wrote:
> Signed-off-by: Alon Levy <al...@redhat.com>
> ---
> cpus.c | 4 ++--
> error.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> error.h | 4 ++++
> qerror.c | 36 ++++++++++++++++++++++++++++++++++++
> qerror.h | 27 +++++++++++++++++++++++++++
> qga/commands-posix.c | 2 +-
> 6 files changed, 114 insertions(+), 3 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 17b055f..c87035f 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1178,7 +1178,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char
> *filename,
>
> f = fopen(filename, "wb");
> if (!f) {
> - error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> + error_set_file_open_failed(errp, filename, errno);
> return;
> }
>
> @@ -1208,7 +1208,7 @@ void qmp_pmemsave(int64_t addr, int64_t size, const
> char *filename,
>
> f = fopen(filename, "wb");
> if (!f) {
> - error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> + error_set_file_open_failed(errp, filename, errno);
> return;
> }
>
> diff --git a/error.c b/error.c
> index 990050f..b32d7f5 100644
> --- a/error.c
> +++ b/error.c
> @@ -144,3 +144,47 @@ void error_set_qobject(Error **errp, QObject *obj)
>
> *errp = err;
> }
> +
> +void error_set_file_open_failed(Error **errp, const char *file_name, int ret)
> +{
I prefer having:
void qemu_fopen_err(const char *filename, const char *mode, Error **errp);
It could live in osdep.c I think.
Also, please split this into two patches:
1. Add all the errors
2. Add qemu_fopen_err()
And don't change unrelated code. Other commands that will benefit from the
new function should be converted in a different series.
CC'ing Anthony in case he has a different suggestion for the fopen()
wrapper (of course we can an open() version too).
> + const char *fmt = NULL;
> +
> + switch (ret) {
> + case EACCES:
> + fmt = QERR_OPEN_FILE_EACCES;
> + break;
> + case EINTR:
> + fmt = QERR_OPEN_FILE_EINTR;
> + break;
> + case EEXIST:
> + fmt = QERR_OPEN_FILE_EEXIST;
> + break;
> + case EMFILE:
> + fmt = QERR_OPEN_FILE_EMFILE;
> + break;
> + case ENOSPC:
> + fmt = QERR_OPEN_FILE_ENOSPC;
> + break;
> + case EPERM:
> + fmt = QERR_OPEN_FILE_EPERM;
> + break;
> + case EROFS:
> + fmt = QERR_OPEN_FILE_EROFS;
> + break;
> + case ENOTDIR:
> + fmt = QERR_OPEN_FILE_ENOTDIR;
> + break;
> + case EFBIG:
> + fmt = QERR_OPEN_FILE_EFBIG;
> + break;
> + default:
> + /*
> + * EINVAL and ENOTSUP will result in the default
> + *
> + * ENOENT too, it's used by (for instance) bdrv_create_file for
> + * a different purpose then open (2) so just give a generic error.
> + */
> + fmt = QERR_OPEN_FILE_FAILED;
> + }
> + error_set(errp, fmt, file_name);
> +}
> diff --git a/error.h b/error.h
> index 6361f40..f3a80f3 100644
> --- a/error.h
> +++ b/error.h
> @@ -67,4 +67,8 @@ void error_free(Error *err);
> */
> bool error_is_type(Error *err, const char *fmt);
>
> +/**
> + * Helper to set error for a open (2) style return code.
> + */
> +void error_set_file_open_failed(Error **errp, const char *file_name, int
> ret);
> #endif
> diff --git a/qerror.c b/qerror.c
> index f55d435..23c260b 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -213,6 +213,42 @@ static const QErrorStringTable qerror_table[] = {
> .desc = "Could not open '%(filename)'",
> },
> {
> + .error_fmt = QERR_OPEN_FILE_EINTR,
> + .desc = "Interrupted open of '%(filename)'",
> + },
I see that you're adding _OPEN_FILE_ flavors to be able to pass the filename
in the error. But this just workarounds a limitation of qerror: we should be
able to set the error message at run-time.
I think the best thing to do is to have QERR_EINTR instead, with a generic
message like "Operation has been interrupted" and we can improve QError to
extend it to contain the file name.
This is valid for all errors below.
> + {
> + .error_fmt = QERR_OPEN_FILE_EACCES,
> + .desc = "Cannot access '%(filename)'",
> + },
We already have PermissionDenied.
> + {
> + .error_fmt = QERR_OPEN_FILE_EEXIST,
> + .desc = "File already exists '%(filename)'",
> + },
I wouldn't mind not having this one because most of the time qemu doesn't
care if the file already exists.
> + {
> + .error_fmt = QERR_OPEN_FILE_EMFILE,
> + .desc = "Max open files when opening '%(filename)'",
> + },
This is an exception for what I said about _OPEN_FILE_ flavors above, this
is fine because it's really a file error.
> + {
> + .error_fmt = QERR_OPEN_FILE_ENOSPC,
> + .desc = "No space left opening '%(filename)'",
> + },
> + {
> + .error_fmt = QERR_OPEN_FILE_EPERM,
> + .desc = "Permission denied (EPERM) for '%(filename)'",
> + },
I wouldn't mind reporting EACCESS and EPERM the same.
> + {
> + .error_fmt = QERR_OPEN_FILE_EROFS,
> + .desc = "Read only filesystem opening '%(filename)'",
> + },
Let's have a QERR_READ_ONLY.
> + {
> + .error_fmt = QERR_OPEN_FILE_ENOTDIR,
> + .desc = "Directory related error opening '%(filename)'",
> + },
> + {
> + .error_fmt = QERR_OPEN_FILE_EFBIG,
> + .desc = "File too big opening '%(filename)'",
> + },
> + {
> .error_fmt = QERR_PERMISSION_DENIED,
> .desc = "Insufficient permission to perform this operation",
> },
> diff --git a/qerror.h b/qerror.h
> index e26c635..6ab9b8d 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_EINTR \
> + "{ 'class': 'OpenFileEINTR', 'data': { 'filename': %s } }"
> +
> +#define QERR_OPEN_FILE_EACCES \
> + "{ 'class': 'OpenFileEACCES', 'data': { 'filename': %s } }"
> +
> +#define QERR_OPEN_FILE_EEXIST \
> + "{ 'class': 'OpenFileEEXIST', 'data': { 'filename': %s } }"
> +
> +#define QERR_OPEN_FILE_EMFILE \
> + "{ 'class': 'OpenFileEMFILE', 'data': { 'filename': %s } }"
> +
> +#define QERR_OPEN_FILE_ENOSPC \
> + "{ 'class': 'OpenFileENOSPC', 'data': { 'filename': %s } }"
> +
> +#define QERR_OPEN_FILE_EPERM \
> + "{ 'class': 'OpenFileEPERM', 'data': { 'filename': %s } }"
> +
> +#define QERR_OPEN_FILE_EROFS \
> + "{ 'class': 'OpenFileEROFS', 'data': { 'filename': %s } }"
> +
> +#define QERR_OPEN_FILE_ENOTDIR \
> + "{ 'class': 'OpenFileENOTDIR', 'data': { 'filename': %s } }"
> +
> +#define QERR_OPEN_FILE_EFBIG \
> + "{ 'class': 'OpenFileEFBIG', 'data': { 'filename': %s } }"
> +
> #define QERR_PERMISSION_DENIED \
> "{ 'class': 'PermissionDenied', 'data': {} }"
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7b2be2f..4e5ef7f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -134,7 +134,7 @@ int64_t qmp_guest_file_open(const char *path, bool
> has_mode, const char *mode, E
> slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
> fh = fopen(path, mode);
> if (!fh) {
> - error_set(err, QERR_OPEN_FILE_FAILED, path);
> + error_set_file_open_failed(err, path, errno);
> return -1;
> }
>
