| Message ID | 1331765714-26209-2-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 15 Mar 2012 00:55:10 +0200 Alon Levy <alevy@redhat.com> wrote: > Signed-off-by: Alon Levy <alevy@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; > } >
On Thu, Mar 15, 2012 at 11:52:32AM -0300, Luiz Capitulino wrote: > On Thu, 15 Mar 2012 00:55:10 +0200 > Alon Levy <alevy@redhat.com> wrote: > > > Signed-off-by: Alon Levy <alevy@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); OK. > > It could live in osdep.c I think. OK. > > Also, please split this into two patches: > > 1. Add all the errors > 2. Add qemu_fopen_err() OK. > > And don't change unrelated code. Other commands that will benefit from the > new function should be converted in a different series. > OK. > 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. > Sounds like we would not have the filename then, which would be a shame. But I don't mind doing that. > This is valid for all errors below. > > > + { > > + .error_fmt = QERR_OPEN_FILE_EACCES, > > + .desc = "Cannot access '%(filename)'", > > + }, > > We already have PermissionDenied. > Not the same use case - QERR_PERMISSION_DENIED is used for internal errors, not for an open / file related system permission error. > > + { > > + .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. I was really just going over everything open (2) manpage and copying what looked like it made sense. I can drop it. > > > + { > > + .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. They are all open (2) errors, see previous comment :) > > > + { > > + .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. But why do that? they are different at the system level, no reason to merge them. Except for EPERM being a bit cryptic (to me) it doesn't seem to be the same as EACCESS. > > > + { > > + .error_fmt = QERR_OPEN_FILE_EROFS, > > + .desc = "Read only filesystem opening '%(filename)'", > > + }, > > Let's have a QERR_READ_ONLY. OK. > > > + { > > + .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; > > } > > >
On Thu, 15 Mar 2012 17:21:01 +0200 Alon Levy <alevy@redhat.com> wrote: > On Thu, Mar 15, 2012 at 11:52:32AM -0300, Luiz Capitulino wrote: > > On Thu, 15 Mar 2012 00:55:10 +0200 > > Alon Levy <alevy@redhat.com> wrote: > > > > > Signed-off-by: Alon Levy <alevy@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); > > OK. > > > > > It could live in osdep.c I think. > > OK. > > > > > Also, please split this into two patches: > > > > 1. Add all the errors > > 2. Add qemu_fopen_err() > > OK. > > > > > And don't change unrelated code. Other commands that will benefit from the > > new function should be converted in a different series. > > > > OK. > > > 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. > > > > Sounds like we would not have the filename then, which would be a shame. We can have it in the future. > But I don't mind doing that. > > > This is valid for all errors below. > > > > > + { > > > + .error_fmt = QERR_OPEN_FILE_EACCES, > > > + .desc = "Cannot access '%(filename)'", > > > + }, > > > > We already have PermissionDenied. > > > > Not the same use case - QERR_PERMISSION_DENIED is used for internal > errors, not for an open / file related system permission error. No problem. > > > > + { > > > + .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. > > I was really just going over everything open (2) manpage and copying > what looked like it made sense. I can drop it. We should only report possible errors, something like ENXIO is not possible in this case. > > > > > > + { > > > + .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. > > They are all open (2) errors, see previous comment :) > > > > > > + { > > > + .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. > > But why do that? they are different at the system level, no reason to > merge them. Except for EPERM being a bit cryptic (to me) it doesn't seem > to be the same as EACCESS. I'm avoiding having a huge amount of errors w/o strong justification, but this is minor to me, do what you think is best. > > > > > > + { > > > + .error_fmt = QERR_OPEN_FILE_EROFS, > > > + .desc = "Read only filesystem opening '%(filename)'", > > > + }, > > > > Let's have a QERR_READ_ONLY. > > OK. > > > > > > + { > > > + .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; > > > } > > > > > >
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) +{ + 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)'", + }, + { + .error_fmt = QERR_OPEN_FILE_EACCES, + .desc = "Cannot access '%(filename)'", + }, + { + .error_fmt = QERR_OPEN_FILE_EEXIST, + .desc = "File already exists '%(filename)'", + }, + { + .error_fmt = QERR_OPEN_FILE_EMFILE, + .desc = "Max open files when opening '%(filename)'", + }, + { + .error_fmt = QERR_OPEN_FILE_ENOSPC, + .desc = "No space left opening '%(filename)'", + }, + { + .error_fmt = QERR_OPEN_FILE_EPERM, + .desc = "Permission denied (EPERM) for '%(filename)'", + }, + { + .error_fmt = QERR_OPEN_FILE_EROFS, + .desc = "Read only filesystem opening '%(filename)'", + }, + { + .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; }
Signed-off-by: Alon Levy <alevy@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(-)