* [Qemu-devel] [PATCH v2 1/5] error: add error_set_file_open_failed
2012-03-14 22:55 [Qemu-devel] [PATCH v2 0/5] screendump qapi convertion Alon Levy
@ 2012-03-14 22:55 ` Alon Levy
2012-03-15 14:52 ` Luiz Capitulino
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 2/5] vga_hw_screen_dump: add Error** param Alon Levy
` (4 subsequent siblings)
5 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-14 22:55 UTC (permalink / raw)
To: qemu-devel, lcapitulino; +Cc: mlureau
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)
+{
+ 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;
}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/5] error: add error_set_file_open_failed
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 1/5] error: add error_set_file_open_failed Alon Levy
@ 2012-03-15 14:52 ` Luiz Capitulino
2012-03-15 15:21 ` Alon Levy
0 siblings, 1 reply; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-15 14:52 UTC (permalink / raw)
To: Alon Levy; +Cc: mlureau, qemu-devel
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;
> }
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/5] error: add error_set_file_open_failed
2012-03-15 14:52 ` Luiz Capitulino
@ 2012-03-15 15:21 ` Alon Levy
2012-03-15 16:00 ` Luiz Capitulino
0 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-15 15:21 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: mlureau, qemu-devel
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;
> > }
> >
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/5] error: add error_set_file_open_failed
2012-03-15 15:21 ` Alon Levy
@ 2012-03-15 16:00 ` Luiz Capitulino
0 siblings, 0 replies; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-15 16:00 UTC (permalink / raw)
To: Alon Levy; +Cc: mlureau, qemu-devel
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;
> > > }
> > >
> >
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v2 2/5] vga_hw_screen_dump: add Error** param
2012-03-14 22:55 [Qemu-devel] [PATCH v2 0/5] screendump qapi convertion Alon Levy
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 1/5] error: add error_set_file_open_failed Alon Levy
@ 2012-03-14 22:55 ` Alon Levy
2012-03-15 14:54 ` Luiz Capitulino
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 3/5] qapi: convert screendump Alon Levy
` (3 subsequent siblings)
5 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-14 22:55 UTC (permalink / raw)
To: qemu-devel, lcapitulino; +Cc: mlureau
Signed-off-by: Alon Levy <alevy@redhat.com>
---
console.c | 5 +++--
console.h | 6 ++++--
hw/blizzard.c | 3 ++-
hw/g364fb.c | 4 +++-
hw/omap_lcdc.c | 4 +++-
hw/qxl.c | 6 ++++--
hw/tcx.c | 13 +++++++++----
hw/vga.c | 7 +++++--
hw/vmware_vga.c | 6 ++++--
monitor.c | 4 +++-
10 files changed, 40 insertions(+), 18 deletions(-)
diff --git a/console.c b/console.c
index 6a463f5..d3fccf3 100644
--- a/console.c
+++ b/console.c
@@ -24,6 +24,7 @@
#include "qemu-common.h"
#include "console.h"
#include "qemu-timer.h"
+#include "error.h"
//#define DEBUG_CONSOLE
#define DEFAULT_BACKSCROLL 512
@@ -173,7 +174,7 @@ void vga_hw_invalidate(void)
active_console->hw_invalidate(active_console->hw);
}
-void vga_hw_screen_dump(const char *filename)
+void vga_hw_screen_dump(const char *filename, Error **errp)
{
TextConsole *previous_active_console;
bool cswitch;
@@ -187,7 +188,7 @@ void vga_hw_screen_dump(const char *filename)
console_select(0);
}
if (consoles[0] && consoles[0]->hw_screen_dump) {
- consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
+ consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, errp);
} else {
error_report("screen dump not implemented");
}
diff --git a/console.h b/console.h
index 4334db5..caf13f5 100644
--- a/console.h
+++ b/console.h
@@ -6,6 +6,7 @@
#include "notify.h"
#include "monitor.h"
#include "trace.h"
+#include "error.h"
/* keyboard/mouse support */
@@ -343,7 +344,8 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
typedef void (*vga_hw_update_ptr)(void *);
typedef void (*vga_hw_invalidate_ptr)(void *);
-typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
+typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch,
+ Error **errp);
typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
DisplayState *graphic_console_init(vga_hw_update_ptr update,
@@ -354,7 +356,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
void vga_hw_update(void);
void vga_hw_invalidate(void);
-void vga_hw_screen_dump(const char *filename);
+void vga_hw_screen_dump(const char *filename, Error **errp);
void vga_hw_text_update(console_ch_t *chardata);
int is_graphic_console(void);
diff --git a/hw/blizzard.c b/hw/blizzard.c
index c7d844d..76df78c 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -23,6 +23,7 @@
#include "devices.h"
#include "vga_int.h"
#include "pixel_ops.h"
+#include "error.h"
typedef void (*blizzard_fn_t)(uint8_t *, const uint8_t *, unsigned int);
@@ -933,7 +934,7 @@ static void blizzard_update_display(void *opaque)
}
static void blizzard_screen_dump(void *opaque, const char *filename,
- bool cswitch)
+ bool cswitch, Error **errp)
{
BlizzardState *s = (BlizzardState *) opaque;
diff --git a/hw/g364fb.c b/hw/g364fb.c
index 3a0b68f..7774d05 100644
--- a/hw/g364fb.c
+++ b/hw/g364fb.c
@@ -22,6 +22,7 @@
#include "pixel_ops.h"
#include "trace.h"
#include "sysbus.h"
+#include "error.h"
typedef struct G364State {
/* hardware */
@@ -289,7 +290,8 @@ static void g364fb_reset(G364State *s)
g364fb_invalidate_display(s);
}
-static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
G364State *s = opaque;
int y, x;
diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
index f172093..aec7210 100644
--- a/hw/omap_lcdc.c
+++ b/hw/omap_lcdc.c
@@ -20,6 +20,7 @@
#include "console.h"
#include "omap.h"
#include "framebuffer.h"
+#include "error.h"
struct omap_lcd_panel_s {
MemoryRegion *sysmem;
@@ -264,7 +265,8 @@ static int ppm_save(const char *filename, uint8_t *data,
return 0;
}
-static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void omap_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
struct omap_lcd_panel_s *omap_lcd = opaque;
if (cswitch) {
diff --git a/hw/qxl.c b/hw/qxl.c
index e17b0e3..27f27f5 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -23,6 +23,7 @@
#include "qemu-queue.h"
#include "monitor.h"
#include "sysemu.h"
+#include "error.h"
#include "qxl.h"
@@ -1492,7 +1493,8 @@ static void qxl_hw_invalidate(void *opaque)
vga->invalidate(vga);
}
-static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
PCIQXLDevice *qxl = opaque;
VGACommonState *vga = &qxl->vga;
@@ -1504,7 +1506,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
ppm_save(filename, qxl->ssd.ds->surface);
break;
case QXL_MODE_VGA:
- vga->screen_dump(vga, filename, cswitch);
+ vga->screen_dump(vga, filename, cswitch, errp);
break;
default:
break;
diff --git a/hw/tcx.c b/hw/tcx.c
index ac7dcb4..f342a5d 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -26,6 +26,7 @@
#include "pixel_ops.h"
#include "sysbus.h"
#include "qdev-addr.h"
+#include "error.h"
#define MAXX 1024
#define MAXY 768
@@ -56,8 +57,10 @@ typedef struct TCXState {
uint8_t dac_index, dac_state;
} TCXState;
-static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch);
-static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch);
+static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error *errp);
+static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error *errp);
static void tcx_set_dirty(TCXState *s)
{
@@ -574,7 +577,8 @@ static int tcx_init1(SysBusDevice *dev)
return 0;
}
-static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error *errp)
{
TCXState *s = opaque;
FILE *f;
@@ -601,7 +605,8 @@ static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
return;
}
-static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error *errp)
{
TCXState *s = opaque;
FILE *f;
diff --git a/hw/vga.c b/hw/vga.c
index 6dc98f6..79c5c38 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -31,6 +31,7 @@
#include "qemu-timer.h"
#include "xen.h"
#include "trace.h"
+#include "error.h"
//#define DEBUG_VGA
//#define DEBUG_VGA_MEM
@@ -163,7 +164,8 @@ static uint32_t expand4[256];
static uint16_t expand2[256];
static uint8_t expand4to8[16];
-static void vga_screen_dump(void *opaque, const char *filename, bool cswitch);
+static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp);
static void vga_update_memory_access(VGACommonState *s)
{
@@ -2409,7 +2411,8 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
/* save the vga display in a PPM image even if no display is
available */
-static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
VGACommonState *s = opaque;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 142d9f4..6868778 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -26,6 +26,7 @@
#include "console.h"
#include "pci.h"
#include "vmware_vga.h"
+#include "error.h"
#undef VERBOSE
#define HW_RECT_ACCEL
@@ -1003,11 +1004,12 @@ static void vmsvga_invalidate_display(void *opaque)
/* save the vga display in a PPM image even if no display is
available */
-static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
struct vmsvga_state_s *s = opaque;
if (!s->enable) {
- s->vga.screen_dump(&s->vga, filename, cswitch);
+ s->vga.screen_dump(&s->vga, filename, cswitch, errp);
return;
}
diff --git a/monitor.c b/monitor.c
index cbdfbad..79399ab 100644
--- a/monitor.c
+++ b/monitor.c
@@ -895,7 +895,9 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
- vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
+ Error *errp = NULL;
+
+ vga_hw_screen_dump(qdict_get_str(qdict, "filename"), &errp);
return 0;
}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/5] vga_hw_screen_dump: add Error** param
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 2/5] vga_hw_screen_dump: add Error** param Alon Levy
@ 2012-03-15 14:54 ` Luiz Capitulino
2012-03-15 15:22 ` Alon Levy
0 siblings, 1 reply; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-15 14:54 UTC (permalink / raw)
To: Alon Levy; +Cc: mlureau, qemu-devel
On Thu, 15 Mar 2012 00:55:11 +0200
Alon Levy <alevy@redhat.com> wrote:
> Signed-off-by: Alon Levy <alevy@redhat.com>
Please, explain why. More comments at the bottom.
> ---
> console.c | 5 +++--
> console.h | 6 ++++--
> hw/blizzard.c | 3 ++-
> hw/g364fb.c | 4 +++-
> hw/omap_lcdc.c | 4 +++-
> hw/qxl.c | 6 ++++--
> hw/tcx.c | 13 +++++++++----
> hw/vga.c | 7 +++++--
> hw/vmware_vga.c | 6 ++++--
> monitor.c | 4 +++-
> 10 files changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/console.c b/console.c
> index 6a463f5..d3fccf3 100644
> --- a/console.c
> +++ b/console.c
> @@ -24,6 +24,7 @@
> #include "qemu-common.h"
> #include "console.h"
> #include "qemu-timer.h"
> +#include "error.h"
>
> //#define DEBUG_CONSOLE
> #define DEFAULT_BACKSCROLL 512
> @@ -173,7 +174,7 @@ void vga_hw_invalidate(void)
> active_console->hw_invalidate(active_console->hw);
> }
>
> -void vga_hw_screen_dump(const char *filename)
> +void vga_hw_screen_dump(const char *filename, Error **errp)
> {
> TextConsole *previous_active_console;
> bool cswitch;
> @@ -187,7 +188,7 @@ void vga_hw_screen_dump(const char *filename)
> console_select(0);
> }
> if (consoles[0] && consoles[0]->hw_screen_dump) {
> - consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
> + consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, errp);
> } else {
> error_report("screen dump not implemented");
> }
> diff --git a/console.h b/console.h
> index 4334db5..caf13f5 100644
> --- a/console.h
> +++ b/console.h
> @@ -6,6 +6,7 @@
> #include "notify.h"
> #include "monitor.h"
> #include "trace.h"
> +#include "error.h"
>
> /* keyboard/mouse support */
>
> @@ -343,7 +344,8 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
>
> typedef void (*vga_hw_update_ptr)(void *);
> typedef void (*vga_hw_invalidate_ptr)(void *);
> -typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> +typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch,
> + Error **errp);
> typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
>
> DisplayState *graphic_console_init(vga_hw_update_ptr update,
> @@ -354,7 +356,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
>
> void vga_hw_update(void);
> void vga_hw_invalidate(void);
> -void vga_hw_screen_dump(const char *filename);
> +void vga_hw_screen_dump(const char *filename, Error **errp);
> void vga_hw_text_update(console_ch_t *chardata);
>
> int is_graphic_console(void);
> diff --git a/hw/blizzard.c b/hw/blizzard.c
> index c7d844d..76df78c 100644
> --- a/hw/blizzard.c
> +++ b/hw/blizzard.c
> @@ -23,6 +23,7 @@
> #include "devices.h"
> #include "vga_int.h"
> #include "pixel_ops.h"
> +#include "error.h"
>
> typedef void (*blizzard_fn_t)(uint8_t *, const uint8_t *, unsigned int);
>
> @@ -933,7 +934,7 @@ static void blizzard_update_display(void *opaque)
> }
>
> static void blizzard_screen_dump(void *opaque, const char *filename,
> - bool cswitch)
> + bool cswitch, Error **errp)
> {
> BlizzardState *s = (BlizzardState *) opaque;
>
> diff --git a/hw/g364fb.c b/hw/g364fb.c
> index 3a0b68f..7774d05 100644
> --- a/hw/g364fb.c
> +++ b/hw/g364fb.c
> @@ -22,6 +22,7 @@
> #include "pixel_ops.h"
> #include "trace.h"
> #include "sysbus.h"
> +#include "error.h"
>
> typedef struct G364State {
> /* hardware */
> @@ -289,7 +290,8 @@ static void g364fb_reset(G364State *s)
> g364fb_invalidate_display(s);
> }
>
> -static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch)
> +static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error **errp)
> {
> G364State *s = opaque;
> int y, x;
> diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> index f172093..aec7210 100644
> --- a/hw/omap_lcdc.c
> +++ b/hw/omap_lcdc.c
> @@ -20,6 +20,7 @@
> #include "console.h"
> #include "omap.h"
> #include "framebuffer.h"
> +#include "error.h"
>
> struct omap_lcd_panel_s {
> MemoryRegion *sysmem;
> @@ -264,7 +265,8 @@ static int ppm_save(const char *filename, uint8_t *data,
> return 0;
> }
>
> -static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
> +static void omap_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error **errp)
> {
> struct omap_lcd_panel_s *omap_lcd = opaque;
> if (cswitch) {
> diff --git a/hw/qxl.c b/hw/qxl.c
> index e17b0e3..27f27f5 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -23,6 +23,7 @@
> #include "qemu-queue.h"
> #include "monitor.h"
> #include "sysemu.h"
> +#include "error.h"
>
> #include "qxl.h"
>
> @@ -1492,7 +1493,8 @@ static void qxl_hw_invalidate(void *opaque)
> vga->invalidate(vga);
> }
>
> -static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
> +static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error **errp)
> {
> PCIQXLDevice *qxl = opaque;
> VGACommonState *vga = &qxl->vga;
> @@ -1504,7 +1506,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
> ppm_save(filename, qxl->ssd.ds->surface);
> break;
> case QXL_MODE_VGA:
> - vga->screen_dump(vga, filename, cswitch);
> + vga->screen_dump(vga, filename, cswitch, errp);
> break;
> default:
> break;
> diff --git a/hw/tcx.c b/hw/tcx.c
> index ac7dcb4..f342a5d 100644
> --- a/hw/tcx.c
> +++ b/hw/tcx.c
> @@ -26,6 +26,7 @@
> #include "pixel_ops.h"
> #include "sysbus.h"
> #include "qdev-addr.h"
> +#include "error.h"
>
> #define MAXX 1024
> #define MAXY 768
> @@ -56,8 +57,10 @@ typedef struct TCXState {
> uint8_t dac_index, dac_state;
> } TCXState;
>
> -static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch);
> -static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch);
> +static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error *errp);
> +static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error *errp);
>
> static void tcx_set_dirty(TCXState *s)
> {
> @@ -574,7 +577,8 @@ static int tcx_init1(SysBusDevice *dev)
> return 0;
> }
>
> -static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
> +static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error *errp)
> {
> TCXState *s = opaque;
> FILE *f;
> @@ -601,7 +605,8 @@ static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
> return;
> }
>
> -static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch)
> +static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error *errp)
> {
> TCXState *s = opaque;
> FILE *f;
> diff --git a/hw/vga.c b/hw/vga.c
> index 6dc98f6..79c5c38 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -31,6 +31,7 @@
> #include "qemu-timer.h"
> #include "xen.h"
> #include "trace.h"
> +#include "error.h"
>
> //#define DEBUG_VGA
> //#define DEBUG_VGA_MEM
> @@ -163,7 +164,8 @@ static uint32_t expand4[256];
> static uint16_t expand2[256];
> static uint8_t expand4to8[16];
>
> -static void vga_screen_dump(void *opaque, const char *filename, bool cswitch);
> +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error **errp);
>
> static void vga_update_memory_access(VGACommonState *s)
> {
> @@ -2409,7 +2411,8 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>
> /* save the vga display in a PPM image even if no display is
> available */
> -static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
> +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error **errp)
> {
> VGACommonState *s = opaque;
>
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 142d9f4..6868778 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -26,6 +26,7 @@
> #include "console.h"
> #include "pci.h"
> #include "vmware_vga.h"
> +#include "error.h"
>
> #undef VERBOSE
> #define HW_RECT_ACCEL
> @@ -1003,11 +1004,12 @@ static void vmsvga_invalidate_display(void *opaque)
>
> /* save the vga display in a PPM image even if no display is
> available */
> -static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch)
> +static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
> + Error **errp)
> {
> struct vmsvga_state_s *s = opaque;
> if (!s->enable) {
> - s->vga.screen_dump(&s->vga, filename, cswitch);
> + s->vga.screen_dump(&s->vga, filename, cswitch, errp);
> return;
> }
>
> diff --git a/monitor.c b/monitor.c
> index cbdfbad..79399ab 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -895,7 +895,9 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
>
> static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> {
> - vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
> + Error *errp = NULL;
> +
> + vga_hw_screen_dump(qdict_get_str(qdict, "filename"), &errp);
Better to pass NULL, as it communicates this is not used (yet).
> return 0;
> }
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/5] vga_hw_screen_dump: add Error** param
2012-03-15 14:54 ` Luiz Capitulino
@ 2012-03-15 15:22 ` Alon Levy
0 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-15 15:22 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: mlureau, qemu-devel
On Thu, Mar 15, 2012 at 11:54:07AM -0300, Luiz Capitulino wrote:
> On Thu, 15 Mar 2012 00:55:11 +0200
> Alon Levy <alevy@redhat.com> wrote:
>
> > Signed-off-by: Alon Levy <alevy@redhat.com>
>
> Please, explain why. More comments at the bottom.
Sure.
>
> > ---
> > console.c | 5 +++--
> > console.h | 6 ++++--
> > hw/blizzard.c | 3 ++-
> > hw/g364fb.c | 4 +++-
> > hw/omap_lcdc.c | 4 +++-
> > hw/qxl.c | 6 ++++--
> > hw/tcx.c | 13 +++++++++----
> > hw/vga.c | 7 +++++--
> > hw/vmware_vga.c | 6 ++++--
> > monitor.c | 4 +++-
> > 10 files changed, 40 insertions(+), 18 deletions(-)
> >
> > diff --git a/console.c b/console.c
> > index 6a463f5..d3fccf3 100644
> > --- a/console.c
> > +++ b/console.c
> > @@ -24,6 +24,7 @@
> > #include "qemu-common.h"
> > #include "console.h"
> > #include "qemu-timer.h"
> > +#include "error.h"
> >
> > //#define DEBUG_CONSOLE
> > #define DEFAULT_BACKSCROLL 512
> > @@ -173,7 +174,7 @@ void vga_hw_invalidate(void)
> > active_console->hw_invalidate(active_console->hw);
> > }
> >
> > -void vga_hw_screen_dump(const char *filename)
> > +void vga_hw_screen_dump(const char *filename, Error **errp)
> > {
> > TextConsole *previous_active_console;
> > bool cswitch;
> > @@ -187,7 +188,7 @@ void vga_hw_screen_dump(const char *filename)
> > console_select(0);
> > }
> > if (consoles[0] && consoles[0]->hw_screen_dump) {
> > - consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
> > + consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, errp);
> > } else {
> > error_report("screen dump not implemented");
> > }
> > diff --git a/console.h b/console.h
> > index 4334db5..caf13f5 100644
> > --- a/console.h
> > +++ b/console.h
> > @@ -6,6 +6,7 @@
> > #include "notify.h"
> > #include "monitor.h"
> > #include "trace.h"
> > +#include "error.h"
> >
> > /* keyboard/mouse support */
> >
> > @@ -343,7 +344,8 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
> >
> > typedef void (*vga_hw_update_ptr)(void *);
> > typedef void (*vga_hw_invalidate_ptr)(void *);
> > -typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> > +typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch,
> > + Error **errp);
> > typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
> >
> > DisplayState *graphic_console_init(vga_hw_update_ptr update,
> > @@ -354,7 +356,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >
> > void vga_hw_update(void);
> > void vga_hw_invalidate(void);
> > -void vga_hw_screen_dump(const char *filename);
> > +void vga_hw_screen_dump(const char *filename, Error **errp);
> > void vga_hw_text_update(console_ch_t *chardata);
> >
> > int is_graphic_console(void);
> > diff --git a/hw/blizzard.c b/hw/blizzard.c
> > index c7d844d..76df78c 100644
> > --- a/hw/blizzard.c
> > +++ b/hw/blizzard.c
> > @@ -23,6 +23,7 @@
> > #include "devices.h"
> > #include "vga_int.h"
> > #include "pixel_ops.h"
> > +#include "error.h"
> >
> > typedef void (*blizzard_fn_t)(uint8_t *, const uint8_t *, unsigned int);
> >
> > @@ -933,7 +934,7 @@ static void blizzard_update_display(void *opaque)
> > }
> >
> > static void blizzard_screen_dump(void *opaque, const char *filename,
> > - bool cswitch)
> > + bool cswitch, Error **errp)
> > {
> > BlizzardState *s = (BlizzardState *) opaque;
> >
> > diff --git a/hw/g364fb.c b/hw/g364fb.c
> > index 3a0b68f..7774d05 100644
> > --- a/hw/g364fb.c
> > +++ b/hw/g364fb.c
> > @@ -22,6 +22,7 @@
> > #include "pixel_ops.h"
> > #include "trace.h"
> > #include "sysbus.h"
> > +#include "error.h"
> >
> > typedef struct G364State {
> > /* hardware */
> > @@ -289,7 +290,8 @@ static void g364fb_reset(G364State *s)
> > g364fb_invalidate_display(s);
> > }
> >
> > -static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch)
> > +static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error **errp)
> > {
> > G364State *s = opaque;
> > int y, x;
> > diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> > index f172093..aec7210 100644
> > --- a/hw/omap_lcdc.c
> > +++ b/hw/omap_lcdc.c
> > @@ -20,6 +20,7 @@
> > #include "console.h"
> > #include "omap.h"
> > #include "framebuffer.h"
> > +#include "error.h"
> >
> > struct omap_lcd_panel_s {
> > MemoryRegion *sysmem;
> > @@ -264,7 +265,8 @@ static int ppm_save(const char *filename, uint8_t *data,
> > return 0;
> > }
> >
> > -static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
> > +static void omap_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error **errp)
> > {
> > struct omap_lcd_panel_s *omap_lcd = opaque;
> > if (cswitch) {
> > diff --git a/hw/qxl.c b/hw/qxl.c
> > index e17b0e3..27f27f5 100644
> > --- a/hw/qxl.c
> > +++ b/hw/qxl.c
> > @@ -23,6 +23,7 @@
> > #include "qemu-queue.h"
> > #include "monitor.h"
> > #include "sysemu.h"
> > +#include "error.h"
> >
> > #include "qxl.h"
> >
> > @@ -1492,7 +1493,8 @@ static void qxl_hw_invalidate(void *opaque)
> > vga->invalidate(vga);
> > }
> >
> > -static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
> > +static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error **errp)
> > {
> > PCIQXLDevice *qxl = opaque;
> > VGACommonState *vga = &qxl->vga;
> > @@ -1504,7 +1506,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
> > ppm_save(filename, qxl->ssd.ds->surface);
> > break;
> > case QXL_MODE_VGA:
> > - vga->screen_dump(vga, filename, cswitch);
> > + vga->screen_dump(vga, filename, cswitch, errp);
> > break;
> > default:
> > break;
> > diff --git a/hw/tcx.c b/hw/tcx.c
> > index ac7dcb4..f342a5d 100644
> > --- a/hw/tcx.c
> > +++ b/hw/tcx.c
> > @@ -26,6 +26,7 @@
> > #include "pixel_ops.h"
> > #include "sysbus.h"
> > #include "qdev-addr.h"
> > +#include "error.h"
> >
> > #define MAXX 1024
> > #define MAXY 768
> > @@ -56,8 +57,10 @@ typedef struct TCXState {
> > uint8_t dac_index, dac_state;
> > } TCXState;
> >
> > -static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch);
> > -static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch);
> > +static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error *errp);
> > +static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error *errp);
> >
> > static void tcx_set_dirty(TCXState *s)
> > {
> > @@ -574,7 +577,8 @@ static int tcx_init1(SysBusDevice *dev)
> > return 0;
> > }
> >
> > -static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
> > +static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error *errp)
> > {
> > TCXState *s = opaque;
> > FILE *f;
> > @@ -601,7 +605,8 @@ static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
> > return;
> > }
> >
> > -static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch)
> > +static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error *errp)
> > {
> > TCXState *s = opaque;
> > FILE *f;
> > diff --git a/hw/vga.c b/hw/vga.c
> > index 6dc98f6..79c5c38 100644
> > --- a/hw/vga.c
> > +++ b/hw/vga.c
> > @@ -31,6 +31,7 @@
> > #include "qemu-timer.h"
> > #include "xen.h"
> > #include "trace.h"
> > +#include "error.h"
> >
> > //#define DEBUG_VGA
> > //#define DEBUG_VGA_MEM
> > @@ -163,7 +164,8 @@ static uint32_t expand4[256];
> > static uint16_t expand2[256];
> > static uint8_t expand4to8[16];
> >
> > -static void vga_screen_dump(void *opaque, const char *filename, bool cswitch);
> > +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error **errp);
> >
> > static void vga_update_memory_access(VGACommonState *s)
> > {
> > @@ -2409,7 +2411,8 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
> >
> > /* save the vga display in a PPM image even if no display is
> > available */
> > -static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
> > +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error **errp)
> > {
> > VGACommonState *s = opaque;
> >
> > diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> > index 142d9f4..6868778 100644
> > --- a/hw/vmware_vga.c
> > +++ b/hw/vmware_vga.c
> > @@ -26,6 +26,7 @@
> > #include "console.h"
> > #include "pci.h"
> > #include "vmware_vga.h"
> > +#include "error.h"
> >
> > #undef VERBOSE
> > #define HW_RECT_ACCEL
> > @@ -1003,11 +1004,12 @@ static void vmsvga_invalidate_display(void *opaque)
> >
> > /* save the vga display in a PPM image even if no display is
> > available */
> > -static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch)
> > +static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
> > + Error **errp)
> > {
> > struct vmsvga_state_s *s = opaque;
> > if (!s->enable) {
> > - s->vga.screen_dump(&s->vga, filename, cswitch);
> > + s->vga.screen_dump(&s->vga, filename, cswitch, errp);
> > return;
> > }
> >
> > diff --git a/monitor.c b/monitor.c
> > index cbdfbad..79399ab 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -895,7 +895,9 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
> >
> > static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > {
> > - vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
> > + Error *errp = NULL;
> > +
> > + vga_hw_screen_dump(qdict_get_str(qdict, "filename"), &errp);
>
> Better to pass NULL, as it communicates this is not used (yet).
OK, in that case I'll check that it actually handles that case correctly
(the point of passing a valid pointer was to not worry about that right
now).
>
> > return 0;
> > }
> >
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v2 3/5] qapi: convert screendump
2012-03-14 22:55 [Qemu-devel] [PATCH v2 0/5] screendump qapi convertion Alon Levy
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 1/5] error: add error_set_file_open_failed Alon Levy
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 2/5] vga_hw_screen_dump: add Error** param Alon Levy
@ 2012-03-14 22:55 ` Alon Levy
2012-03-15 14:55 ` Luiz Capitulino
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 4/5] vga: ppm_save(): Return error on failure Alon Levy
` (2 subsequent siblings)
5 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-14 22:55 UTC (permalink / raw)
To: qemu-devel, lcapitulino; +Cc: mlureau
Signed-off-by: Alon Levy <alevy@redhat.com>
---
hmp-commands.hx | 3 +--
hmp.c | 8 ++++++++
hmp.h | 1 +
monitor.c | 8 --------
qapi-schema.json | 13 +++++++++++++
qmp-commands.hx | 5 +----
qmp.c | 5 +++++
7 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 6980214..d26421a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -194,8 +194,7 @@ ETEXI
.args_type = "filename:F",
.params = "filename",
.help = "save screen into PPM image 'filename'",
- .user_print = monitor_user_noop,
- .mhandler.cmd_new = do_screen_dump,
+ .mhandler.cmd = hmp_screendump,
},
STEXI
diff --git a/hmp.c b/hmp.c
index 290c43d..42dc79a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -860,3 +860,11 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &error);
}
+
+void hmp_screendump(Monitor *mon, const QDict *qdict)
+{
+ Error *err = NULL;
+
+ qmp_screendump(qdict_get_str(qdict, "filename"), &err);
+ hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 5409464..25d123f 100644
--- a/hmp.h
+++ b/hmp.h
@@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
void hmp_block_stream(Monitor *mon, const QDict *qdict);
void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
+void hmp_screendump(Monitor *mon, const QDict *qdict);
#endif
diff --git a/monitor.c b/monitor.c
index 79399ab..f79ce9a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -893,14 +893,6 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
return -1;
}
-static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
-{
- Error *errp = NULL;
-
- vga_hw_screen_dump(qdict_get_str(qdict, "filename"), &errp);
- return 0;
-}
-
static void do_logfile(Monitor *mon, const QDict *qdict)
{
cpu_set_log_filename(qdict_get_str(qdict, "filename"));
diff --git a/qapi-schema.json b/qapi-schema.json
index 04fa84f..4f251ca 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1663,3 +1663,16 @@
{ 'command': 'qom-list-types',
'data': { '*implements': 'str', '*abstract': 'bool' },
'returns': [ 'ObjectTypeInfo' ] }
+
+##
+# @screendump:
+#
+# Write a PPM of the VGA screen to a file.
+#
+# @filename: the name of a new PPM file to create to store the image
+#
+# Returns: Nothing on success
+#
+# Since: 1.1
+##
+{ 'command': 'screendump', 'data': {'filename': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index dfe8a5b..5fe57fd 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -146,10 +146,7 @@ EQMP
{
.name = "screendump",
.args_type = "filename:F",
- .params = "filename",
- .help = "save screen into PPM image 'filename'",
- .user_print = monitor_user_noop,
- .mhandler.cmd_new = do_screen_dump,
+ .mhandler.cmd_new = qmp_marshal_input_screendump,
},
SQMP
diff --git a/qmp.c b/qmp.c
index a182b51..086cec8 100644
--- a/qmp.c
+++ b/qmp.c
@@ -415,3 +415,8 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
return ret;
}
+
+void qmp_screendump(const char *filename, Error **errp)
+{
+ vga_hw_screen_dump(filename, errp);
+}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/5] qapi: convert screendump
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 3/5] qapi: convert screendump Alon Levy
@ 2012-03-15 14:55 ` Luiz Capitulino
2012-03-15 15:23 ` Alon Levy
0 siblings, 1 reply; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-15 14:55 UTC (permalink / raw)
To: Alon Levy; +Cc: mlureau, qemu-devel
On Thu, 15 Mar 2012 00:55:12 +0200
Alon Levy <alevy@redhat.com> wrote:
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
> hmp-commands.hx | 3 +--
> hmp.c | 8 ++++++++
> hmp.h | 1 +
> monitor.c | 8 --------
> qapi-schema.json | 13 +++++++++++++
> qmp-commands.hx | 5 +----
> qmp.c | 5 +++++
> 7 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 6980214..d26421a 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -194,8 +194,7 @@ ETEXI
> .args_type = "filename:F",
> .params = "filename",
> .help = "save screen into PPM image 'filename'",
> - .user_print = monitor_user_noop,
> - .mhandler.cmd_new = do_screen_dump,
> + .mhandler.cmd = hmp_screendump,
> },
>
> STEXI
> diff --git a/hmp.c b/hmp.c
> index 290c43d..42dc79a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -860,3 +860,11 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
>
> hmp_handle_error(mon, &error);
> }
> +
> +void hmp_screendump(Monitor *mon, const QDict *qdict)
> +{
> + Error *err = NULL;
> +
> + qmp_screendump(qdict_get_str(qdict, "filename"), &err);
> + hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 5409464..25d123f 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
> void hmp_block_stream(Monitor *mon, const QDict *qdict);
> void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
> void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> +void hmp_screendump(Monitor *mon, const QDict *qdict);
>
> #endif
> diff --git a/monitor.c b/monitor.c
> index 79399ab..f79ce9a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -893,14 +893,6 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
> return -1;
> }
>
> -static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> -{
> - Error *errp = NULL;
> -
> - vga_hw_screen_dump(qdict_get_str(qdict, "filename"), &errp);
> - return 0;
> -}
> -
> static void do_logfile(Monitor *mon, const QDict *qdict)
> {
> cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 04fa84f..4f251ca 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1663,3 +1663,16 @@
> { 'command': 'qom-list-types',
> 'data': { '*implements': 'str', '*abstract': 'bool' },
> 'returns': [ 'ObjectTypeInfo' ] }
> +
> +##
> +# @screendump:
> +#
> +# Write a PPM of the VGA screen to a file.
> +#
> +# @filename: the name of a new PPM file to create to store the image
> +#
> +# Returns: Nothing on success
Note that you have to document all the _possible_ errors. Look for other
examples in this file.
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'screendump', 'data': {'filename': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index dfe8a5b..5fe57fd 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -146,10 +146,7 @@ EQMP
> {
> .name = "screendump",
> .args_type = "filename:F",
> - .params = "filename",
> - .help = "save screen into PPM image 'filename'",
> - .user_print = monitor_user_noop,
> - .mhandler.cmd_new = do_screen_dump,
> + .mhandler.cmd_new = qmp_marshal_input_screendump,
> },
>
> SQMP
> diff --git a/qmp.c b/qmp.c
> index a182b51..086cec8 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -415,3 +415,8 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>
> return ret;
> }
> +
> +void qmp_screendump(const char *filename, Error **errp)
> +{
> + vga_hw_screen_dump(filename, errp);
> +}
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/5] qapi: convert screendump
2012-03-15 14:55 ` Luiz Capitulino
@ 2012-03-15 15:23 ` Alon Levy
0 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-15 15:23 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: mlureau, qemu-devel
On Thu, Mar 15, 2012 at 11:55:31AM -0300, Luiz Capitulino wrote:
> On Thu, 15 Mar 2012 00:55:12 +0200
> Alon Levy <alevy@redhat.com> wrote:
>
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> > hmp-commands.hx | 3 +--
> > hmp.c | 8 ++++++++
> > hmp.h | 1 +
> > monitor.c | 8 --------
> > qapi-schema.json | 13 +++++++++++++
> > qmp-commands.hx | 5 +----
> > qmp.c | 5 +++++
> > 7 files changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 6980214..d26421a 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -194,8 +194,7 @@ ETEXI
> > .args_type = "filename:F",
> > .params = "filename",
> > .help = "save screen into PPM image 'filename'",
> > - .user_print = monitor_user_noop,
> > - .mhandler.cmd_new = do_screen_dump,
> > + .mhandler.cmd = hmp_screendump,
> > },
> >
> > STEXI
> > diff --git a/hmp.c b/hmp.c
> > index 290c43d..42dc79a 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -860,3 +860,11 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
> >
> > hmp_handle_error(mon, &error);
> > }
> > +
> > +void hmp_screendump(Monitor *mon, const QDict *qdict)
> > +{
> > + Error *err = NULL;
> > +
> > + qmp_screendump(qdict_get_str(qdict, "filename"), &err);
> > + hmp_handle_error(mon, &err);
> > +}
> > diff --git a/hmp.h b/hmp.h
> > index 5409464..25d123f 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
> > void hmp_block_stream(Monitor *mon, const QDict *qdict);
> > void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
> > void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> > +void hmp_screendump(Monitor *mon, const QDict *qdict);
> >
> > #endif
> > diff --git a/monitor.c b/monitor.c
> > index 79399ab..f79ce9a 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -893,14 +893,6 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
> > return -1;
> > }
> >
> > -static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > -{
> > - Error *errp = NULL;
> > -
> > - vga_hw_screen_dump(qdict_get_str(qdict, "filename"), &errp);
> > - return 0;
> > -}
> > -
> > static void do_logfile(Monitor *mon, const QDict *qdict)
> > {
> > cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 04fa84f..4f251ca 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1663,3 +1663,16 @@
> > { 'command': 'qom-list-types',
> > 'data': { '*implements': 'str', '*abstract': 'bool' },
> > 'returns': [ 'ObjectTypeInfo' ] }
> > +
> > +##
> > +# @screendump:
> > +#
> > +# Write a PPM of the VGA screen to a file.
> > +#
> > +# @filename: the name of a new PPM file to create to store the image
> > +#
> > +# Returns: Nothing on success
>
> Note that you have to document all the _possible_ errors. Look for other
> examples in this file.
Yes, forgot about that. Another reason to split the other users of
qerr_fopen_err then :)
>
> > +#
> > +# Since: 1.1
> > +##
> > +{ 'command': 'screendump', 'data': {'filename': 'str'} }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index dfe8a5b..5fe57fd 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -146,10 +146,7 @@ EQMP
> > {
> > .name = "screendump",
> > .args_type = "filename:F",
> > - .params = "filename",
> > - .help = "save screen into PPM image 'filename'",
> > - .user_print = monitor_user_noop,
> > - .mhandler.cmd_new = do_screen_dump,
> > + .mhandler.cmd_new = qmp_marshal_input_screendump,
> > },
> >
> > SQMP
> > diff --git a/qmp.c b/qmp.c
> > index a182b51..086cec8 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -415,3 +415,8 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
> >
> > return ret;
> > }
> > +
> > +void qmp_screendump(const char *filename, Error **errp)
> > +{
> > + vga_hw_screen_dump(filename, errp);
> > +}
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v2 4/5] vga: ppm_save(): Return error on failure
2012-03-14 22:55 [Qemu-devel] [PATCH v2 0/5] screendump qapi convertion Alon Levy
` (2 preceding siblings ...)
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 3/5] qapi: convert screendump Alon Levy
@ 2012-03-14 22:55 ` Alon Levy
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 5/5] blockdev: use error_set_file_open_failed Alon Levy
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 0/5] screendump qapi convertion Alon Levy
5 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-14 22:55 UTC (permalink / raw)
To: qemu-devel, lcapitulino; +Cc: mlureau
From: Luiz Capitulino <lcapitulino@redhat.com>
This makes all devices using ppm_save() return an error appropriately
when the screendump command fails.
Based on a code by Anthony Liguori.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Alon Levy <alevy@redhat.com>
---
hw/blizzard.c | 2 +-
hw/qxl.c | 2 +-
hw/vga.c | 8 +++++---
hw/vga_int.h | 3 ++-
hw/vmware_vga.c | 2 +-
5 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/hw/blizzard.c b/hw/blizzard.c
index 76df78c..29e5ae6 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -942,7 +942,7 @@ static void blizzard_screen_dump(void *opaque, const char *filename,
blizzard_update_display(opaque);
}
if (s && ds_get_data(s->state))
- ppm_save(filename, s->state->surface);
+ ppm_save(filename, s->state->surface, errp);
}
#define DEPTH 8
diff --git a/hw/qxl.c b/hw/qxl.c
index 27f27f5..aa68612 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1503,7 +1503,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
case QXL_MODE_COMPAT:
case QXL_MODE_NATIVE:
qxl_render_update(qxl);
- ppm_save(filename, qxl->ssd.ds->surface);
+ ppm_save(filename, qxl->ssd.ds->surface, errp);
break;
case QXL_MODE_VGA:
vga->screen_dump(vga, filename, cswitch, errp);
diff --git a/hw/vga.c b/hw/vga.c
index 79c5c38..80e6dca 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2365,7 +2365,7 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
/********************************************************/
/* vga screen dump */
-int ppm_save(const char *filename, struct DisplaySurface *ds)
+int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp)
{
FILE *f;
uint8_t *d, *d1;
@@ -2377,8 +2377,10 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
trace_ppm_save(filename, ds);
f = fopen(filename, "wb");
- if (!f)
+ if (!f) {
+ error_set_file_open_failed(errp, filename, errno);
return -1;
+ }
fprintf(f, "P6\n%d %d\n%d\n",
ds->width, ds->height, 255);
linebuf = g_malloc(ds->width * 3);
@@ -2420,5 +2422,5 @@ static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
vga_invalidate_display(s);
vga_hw_update();
}
- ppm_save(filename, s->ds->surface);
+ ppm_save(filename, s->ds->surface, errp);
}
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 7685b2b..63078ba 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -24,6 +24,7 @@
#include <hw/hw.h>
#include "memory.h"
+#include "error.h"
#define ST01_V_RETRACE 0x08
#define ST01_DISP_ENABLE 0x01
@@ -200,7 +201,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val);
uint32_t vga_mem_readb(VGACommonState *s, target_phys_addr_t addr);
void vga_mem_writeb(VGACommonState *s, target_phys_addr_t addr, uint32_t val);
void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
-int ppm_save(const char *filename, struct DisplaySurface *ds);
+int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp);
int vga_ioport_invalid(VGACommonState *s, uint32_t addr);
void vga_init_vbe(VGACommonState *s, MemoryRegion *address_space);
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 6868778..0769652 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1016,7 +1016,7 @@ static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
if (s->depth == 32) {
DisplaySurface *ds = qemu_create_displaysurface_from(s->width,
s->height, 32, ds_get_linesize(s->vga.ds), s->vga.vram_ptr);
- ppm_save(filename, ds);
+ ppm_save(filename, ds, errp);
g_free(ds);
}
}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v2 5/5] blockdev: use error_set_file_open_failed
2012-03-14 22:55 [Qemu-devel] [PATCH v2 0/5] screendump qapi convertion Alon Levy
` (3 preceding siblings ...)
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 4/5] vga: ppm_save(): Return error on failure Alon Levy
@ 2012-03-14 22:55 ` Alon Levy
2012-03-15 14:56 ` Luiz Capitulino
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 0/5] screendump qapi convertion Alon Levy
5 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-14 22:55 UTC (permalink / raw)
To: qemu-devel, lcapitulino; +Cc: mlureau
This is a little trickier, since those calls chain in many fun ways and
produce sometimes their own return values reusing existing errno values
for similar meanings. In that respect error_set_file_open_failed
specifically ignores EINVAL, ENOTSUP and ENOENT. The first two simply
are not returned by open (2), but the last is but I chose to ignore it
to allow easy reuse in blockdev to avoid confusion when it is used
internally by the create functions.
Signed-off-by: Alon Levy <alevy@redhat.com>
---
blockdev.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 1a500b8..544d067 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -777,7 +777,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
states->old_bs->drv->format_name,
NULL, -1, flags);
if (ret) {
- error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+ error_set_file_open_failed(errp, new_image_file, -ret);
goto delete_and_fail;
}
}
@@ -787,7 +787,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
ret = bdrv_open(states->new_bs, new_image_file,
flags | BDRV_O_NO_BACKING, drv);
if (ret != 0) {
- error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+ error_set_file_open_failed(errp, new_image_file, -ret);
goto delete_and_fail;
}
}
@@ -881,8 +881,11 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
int bdrv_flags, BlockDriver *drv,
const char *password, Error **errp)
{
- if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
- error_set(errp, QERR_OPEN_FILE_FAILED, filename);
+ int ret;
+
+ ret = bdrv_open(bs, filename, bdrv_flags, drv);
+ if (ret < 0) {
+ error_set_file_open_failed(errp, filename, ret);
return;
}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/5] blockdev: use error_set_file_open_failed
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 5/5] blockdev: use error_set_file_open_failed Alon Levy
@ 2012-03-15 14:56 ` Luiz Capitulino
2012-03-15 15:23 ` Alon Levy
0 siblings, 1 reply; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-15 14:56 UTC (permalink / raw)
To: Alon Levy; +Cc: mlureau, qemu-devel
On Thu, 15 Mar 2012 00:55:14 +0200
Alon Levy <alevy@redhat.com> wrote:
> This is a little trickier, since those calls chain in many fun ways and
> produce sometimes their own return values reusing existing errno values
> for similar meanings. In that respect error_set_file_open_failed
> specifically ignores EINVAL, ENOTSUP and ENOENT. The first two simply
> are not returned by open (2), but the last is but I chose to ignore it
> to allow easy reuse in blockdev to avoid confusion when it is used
> internally by the create functions.
Please, just drop this from this series as it's completely unrelated to
the screendump command.
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
> blockdev.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 1a500b8..544d067 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -777,7 +777,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
> states->old_bs->drv->format_name,
> NULL, -1, flags);
> if (ret) {
> - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> + error_set_file_open_failed(errp, new_image_file, -ret);
> goto delete_and_fail;
> }
> }
> @@ -787,7 +787,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
> ret = bdrv_open(states->new_bs, new_image_file,
> flags | BDRV_O_NO_BACKING, drv);
> if (ret != 0) {
> - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> + error_set_file_open_failed(errp, new_image_file, -ret);
> goto delete_and_fail;
> }
> }
> @@ -881,8 +881,11 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
> int bdrv_flags, BlockDriver *drv,
> const char *password, Error **errp)
> {
> - if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> - error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> + int ret;
> +
> + ret = bdrv_open(bs, filename, bdrv_flags, drv);
> + if (ret < 0) {
> + error_set_file_open_failed(errp, filename, ret);
> return;
> }
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/5] blockdev: use error_set_file_open_failed
2012-03-15 14:56 ` Luiz Capitulino
@ 2012-03-15 15:23 ` Alon Levy
0 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-15 15:23 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: mlureau, qemu-devel
On Thu, Mar 15, 2012 at 11:56:29AM -0300, Luiz Capitulino wrote:
> On Thu, 15 Mar 2012 00:55:14 +0200
> Alon Levy <alevy@redhat.com> wrote:
>
> > This is a little trickier, since those calls chain in many fun ways and
> > produce sometimes their own return values reusing existing errno values
> > for similar meanings. In that respect error_set_file_open_failed
> > specifically ignores EINVAL, ENOTSUP and ENOENT. The first two simply
> > are not returned by open (2), but the last is but I chose to ignore it
> > to allow easy reuse in blockdev to avoid confusion when it is used
> > internally by the create functions.
>
> Please, just drop this from this series as it's completely unrelated to
> the screendump command.
After I went to all the trouble jumping through blockdev.c ? OK :)
>
> >
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> > blockdev.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 1a500b8..544d067 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -777,7 +777,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
> > states->old_bs->drv->format_name,
> > NULL, -1, flags);
> > if (ret) {
> > - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> > + error_set_file_open_failed(errp, new_image_file, -ret);
> > goto delete_and_fail;
> > }
> > }
> > @@ -787,7 +787,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
> > ret = bdrv_open(states->new_bs, new_image_file,
> > flags | BDRV_O_NO_BACKING, drv);
> > if (ret != 0) {
> > - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> > + error_set_file_open_failed(errp, new_image_file, -ret);
> > goto delete_and_fail;
> > }
> > }
> > @@ -881,8 +881,11 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
> > int bdrv_flags, BlockDriver *drv,
> > const char *password, Error **errp)
> > {
> > - if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> > - error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> > + int ret;
> > +
> > + ret = bdrv_open(bs, filename, bdrv_flags, drv);
> > + if (ret < 0) {
> > + error_set_file_open_failed(errp, filename, ret);
> > return;
> > }
> >
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v3 0/5] screendump qapi convertion
2012-03-14 22:55 [Qemu-devel] [PATCH v2 0/5] screendump qapi convertion Alon Levy
` (4 preceding siblings ...)
2012-03-14 22:55 ` [Qemu-devel] [PATCH v2 5/5] blockdev: use error_set_file_open_failed Alon Levy
@ 2012-03-18 18:29 ` Alon Levy
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure Alon Levy
` (4 more replies)
5 siblings, 5 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-18 18:29 UTC (permalink / raw)
To: qemu-devel, Luiz Capitulino
v3 changes: (Requested by Luiz Capitulino)
rename helper to qemu_fopen_err
don't touch unrelated subsystems
move helper to osdep - I couldn't do it that way without dragging a lot of dependencies into libcacard which uses osdep-obj-y, so I split it so that there is now a osdep-no-qerror-obj-y that is solemnly used by libcacard and is contained in osdep-obj-y.
rename some of the QERR codes, specifically most of them lost the OPEN_FILE substring.
split the first patch in two, adding error codes and adding qemu_fopen_err
v2 changes:
split screendump convertion to an addition "add Error** param"
handle various errors of fopen with new qerror codes
Alon Levy (4):
qerror: add error codes for fopen failure
add qemu_fopen_err
vga_hw_screen_dump: add Error** param
qapi: convert screendump
Luiz Capitulino (1):
vga: ppm_save(): Return error on failure
Makefile.objs | 8 +++++---
console.c | 5 +++--
console.h | 6 ++++--
hmp-commands.hx | 3 +--
hmp.c | 8 ++++++++
hmp.h | 1 +
hw/blizzard.c | 5 +++--
hw/g364fb.c | 4 +++-
hw/omap_lcdc.c | 4 +++-
hw/qxl.c | 8 +++++---
hw/tcx.c | 13 +++++++++----
hw/vga.c | 15 ++++++++++-----
hw/vga_int.h | 3 ++-
hw/vmware_vga.c | 8 +++++---
libcacard/Makefile | 2 +-
monitor.c | 6 ------
osdep-qerror.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
osdep-qerror.h | 8 ++++++++
osdep.c | 1 -
qapi-schema.json | 24 ++++++++++++++++++++++++
qerror.c | 36 ++++++++++++++++++++++++++++++++++++
qerror.h | 27 +++++++++++++++++++++++++++
qmp-commands.hx | 5 +----
qmp.c | 5 +++++
24 files changed, 216 insertions(+), 41 deletions(-)
create mode 100644 osdep-qerror.c
create mode 100644 osdep-qerror.h
--
1.7.9.3
^ permalink raw reply [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 0/5] screendump qapi convertion Alon Levy
@ 2012-03-18 18:29 ` Alon Levy
2012-03-23 14:21 ` Luiz Capitulino
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 2/5] add qemu_fopen_err Alon Levy
` (3 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-18 18:29 UTC (permalink / raw)
To: qemu-devel, Luiz Capitulino
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)'",
+ },
+ {
+ .error_fmt = QERR_EACCES,
+ .desc = "Cannot access file'",
+ },
+ {
+ .error_fmt = QERR_EEXIST,
+ .desc = "File already exists'",
+ },
+ {
+ .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': {} }"
+
+#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': {} }"
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure Alon Levy
@ 2012-03-23 14:21 ` Luiz Capitulino
2012-03-23 19:53 ` Alon Levy
` (2 more replies)
0 siblings, 3 replies; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-23 14:21 UTC (permalink / raw)
To: Alon Levy; +Cc: qemu-devel
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.
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': {} }"
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure
2012-03-23 14:21 ` Luiz Capitulino
@ 2012-03-23 19:53 ` Alon Levy
2012-03-23 20:48 ` Luiz Capitulino
2012-03-26 18:44 ` Alon Levy
2012-03-26 18:51 ` Alon Levy
2 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-23 19:53 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
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? 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.
>
> > + {
> > + .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).
>
> > +
> > +#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': {} }"
> >
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure
2012-03-23 19:53 ` Alon Levy
@ 2012-03-23 20:48 ` Luiz Capitulino
0 siblings, 0 replies; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-23 20:48 UTC (permalink / raw)
To: Alon Levy; +Cc: qemu-devel
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': {} }"
> > >
> >
> >
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure
2012-03-23 14:21 ` Luiz Capitulino
2012-03-23 19:53 ` Alon Levy
@ 2012-03-26 18:44 ` Alon Levy
2012-03-26 18:51 ` Alon Levy
2 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-26 18:44 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
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.
>
> 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().
That would require either blocking or using async monitor... and in
general I guess it makes the signature of qemu_fopen_err be:
void qemu_fopen_err(const char *path, const char *mode, int block,
Error **errp)
Sounds ok?
>
> > + {
> > + .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': {} }"
> >
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure
2012-03-23 14:21 ` Luiz Capitulino
2012-03-23 19:53 ` Alon Levy
2012-03-26 18:44 ` Alon Levy
@ 2012-03-26 18:51 ` Alon Levy
2 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-26 18:51 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
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.
>
> 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': {} }"
> >
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v3 2/5] add qemu_fopen_err
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 0/5] screendump qapi convertion Alon Levy
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure Alon Levy
@ 2012-03-18 18:29 ` Alon Levy
2012-03-23 14:22 ` Luiz Capitulino
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 3/5] vga_hw_screen_dump: add Error** param Alon Levy
` (2 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-18 18:29 UTC (permalink / raw)
To: qemu-devel, Luiz Capitulino
This adds a helper to conveniently set the correct error based on the
errno after a failed fopen.
The added function is placed in it's own c file to allow libcacard to
not develop dependencies on everything that qerror will bring in, which
includes monitor and half of qemu. I tried to make it as less ugly as I
could, by naming an osdep-no-qerror-obj-y and having that included in
osdep-obj-y, and using only the former for libcacard.
Signed-off-by: Alon Levy <alevy@redhat.com>
---
Makefile.objs | 8 +++++---
libcacard/Makefile | 2 +-
osdep-qerror.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
osdep-qerror.h | 8 ++++++++
osdep.c | 1 -
5 files changed, 66 insertions(+), 5 deletions(-)
create mode 100644 osdep-qerror.c
create mode 100644 osdep-qerror.h
diff --git a/Makefile.objs b/Makefile.objs
index 226b01d..fb5a73a 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -20,9 +20,11 @@ universal-obj-y += $(qom-obj-y)
#######################################################################
# oslib-obj-y is code depending on the OS (win32 vs posix)
-oslib-obj-y = osdep.o
-oslib-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o
-oslib-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
+oslib-no-qerror-obj-y = osdep.o
+oslib-no-qerror-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o
+oslib-no-qerror-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
+oslib-obj-y = $(oslib-no-qerror-obj-y)
+oslib-obj-y += osdep-qerror.o
#######################################################################
# coroutines
diff --git a/libcacard/Makefile b/libcacard/Makefile
index c6a896a..83f483f 100644
--- a/libcacard/Makefile
+++ b/libcacard/Makefile
@@ -8,7 +8,7 @@ libcacard_includedir=$(includedir)/cacard
$(call set-vpath, $(SRC_PATH):$(libcacard_srcpath))
# objects linked against normal qemu binaries, not compiled with libtool
-QEMU_OBJS=$(addprefix ../,$(oslib-obj-y) qemu-timer-common.o $(trace-obj-y))
+QEMU_OBJS=$(addprefix ../,$(oslib-no-qerror-obj-y) qemu-timer-common.o $(trace-obj-y))
# objects linked into a shared library, built with libtool with -fPIC if required
QEMU_OBJS_LIB=$(addsuffix .lo,$(basename $(QEMU_OBJS)))
diff --git a/osdep-qerror.c b/osdep-qerror.c
new file mode 100644
index 0000000..6dac984
--- /dev/null
+++ b/osdep-qerror.c
@@ -0,0 +1,52 @@
+#include "qerror.h"
+
+#include "osdep-qerror.h"
+
+/*
+ * Helper to set an Error after a failed fopen.
+ *
+ * Uses errno so it must not be changed by another intermediate call.
+ */
+void qemu_fopen_err(Error **errp, const char *file_name)
+{
+ const char *fmt = NULL;
+
+ switch (errno) {
+ case EACCES:
+ fmt = QERR_EACCES;
+ break;
+ case EINTR:
+ fmt = QERR_EINTR;
+ break;
+ case EEXIST:
+ fmt = QERR_EEXIST;
+ break;
+ case EMFILE:
+ fmt = QERR_OPEN_FILE_EMFILE;
+ break;
+ case ENOSPC:
+ fmt = QERR_ENOSPC;
+ break;
+ case EPERM:
+ fmt = QERR_EPERM;
+ break;
+ case EROFS:
+ fmt = QERR_READ_ONLY;
+ break;
+ case ENOTDIR:
+ fmt = QERR_ENOTDIR;
+ break;
+ case EFBIG:
+ fmt = QERR_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/osdep-qerror.h b/osdep-qerror.h
new file mode 100644
index 0000000..7320f4a
--- /dev/null
+++ b/osdep-qerror.h
@@ -0,0 +1,8 @@
+#ifndef OSDEP_QERROR_H
+#define OSDEP_QERROR_H
+
+#include "error.h"
+
+void qemu_fopen_err(Error **errp, const char *file_name);
+
+#endif
diff --git a/osdep.c b/osdep.c
index 3e6bada..efdd21c 100644
--- a/osdep.c
+++ b/osdep.c
@@ -241,4 +241,3 @@ ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags)
return total;
}
-
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/5] add qemu_fopen_err
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 2/5] add qemu_fopen_err Alon Levy
@ 2012-03-23 14:22 ` Luiz Capitulino
2012-03-23 19:55 ` Alon Levy
0 siblings, 1 reply; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-23 14:22 UTC (permalink / raw)
To: Alon Levy; +Cc: qemu-devel
On Sun, 18 Mar 2012 19:29:10 +0100
Alon Levy <alevy@redhat.com> wrote:
> This adds a helper to conveniently set the correct error based on the
> errno after a failed fopen.
>
> The added function is placed in it's own c file to allow libcacard to
> not develop dependencies on everything that qerror will bring in, which
> includes monitor and half of qemu. I tried to make it as less ugly as I
> could, by naming an osdep-no-qerror-obj-y and having that included in
> osdep-obj-y, and using only the former for libcacard.
I'm not sure I like this, how will libcacard report errors then?
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
> Makefile.objs | 8 +++++---
> libcacard/Makefile | 2 +-
> osdep-qerror.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> osdep-qerror.h | 8 ++++++++
> osdep.c | 1 -
> 5 files changed, 66 insertions(+), 5 deletions(-)
> create mode 100644 osdep-qerror.c
> create mode 100644 osdep-qerror.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 226b01d..fb5a73a 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -20,9 +20,11 @@ universal-obj-y += $(qom-obj-y)
>
> #######################################################################
> # oslib-obj-y is code depending on the OS (win32 vs posix)
> -oslib-obj-y = osdep.o
> -oslib-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o
> -oslib-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
> +oslib-no-qerror-obj-y = osdep.o
> +oslib-no-qerror-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o
> +oslib-no-qerror-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
> +oslib-obj-y = $(oslib-no-qerror-obj-y)
> +oslib-obj-y += osdep-qerror.o
>
> #######################################################################
> # coroutines
> diff --git a/libcacard/Makefile b/libcacard/Makefile
> index c6a896a..83f483f 100644
> --- a/libcacard/Makefile
> +++ b/libcacard/Makefile
> @@ -8,7 +8,7 @@ libcacard_includedir=$(includedir)/cacard
> $(call set-vpath, $(SRC_PATH):$(libcacard_srcpath))
>
> # objects linked against normal qemu binaries, not compiled with libtool
> -QEMU_OBJS=$(addprefix ../,$(oslib-obj-y) qemu-timer-common.o $(trace-obj-y))
> +QEMU_OBJS=$(addprefix ../,$(oslib-no-qerror-obj-y) qemu-timer-common.o $(trace-obj-y))
>
> # objects linked into a shared library, built with libtool with -fPIC if required
> QEMU_OBJS_LIB=$(addsuffix .lo,$(basename $(QEMU_OBJS)))
> diff --git a/osdep-qerror.c b/osdep-qerror.c
> new file mode 100644
> index 0000000..6dac984
> --- /dev/null
> +++ b/osdep-qerror.c
> @@ -0,0 +1,52 @@
> +#include "qerror.h"
> +
> +#include "osdep-qerror.h"
> +
> +/*
> + * Helper to set an Error after a failed fopen.
> + *
> + * Uses errno so it must not be changed by another intermediate call.
> + */
> +void qemu_fopen_err(Error **errp, const char *file_name)
> +{
> + const char *fmt = NULL;
Where's the fopen() call and the mode argument?
> +
> + switch (errno) {
> + case EACCES:
> + fmt = QERR_EACCES;
> + break;
> + case EINTR:
> + fmt = QERR_EINTR;
> + break;
> + case EEXIST:
> + fmt = QERR_EEXIST;
> + break;
> + case EMFILE:
> + fmt = QERR_OPEN_FILE_EMFILE;
> + break;
> + case ENOSPC:
> + fmt = QERR_ENOSPC;
> + break;
> + case EPERM:
> + fmt = QERR_EPERM;
> + break;
> + case EROFS:
> + fmt = QERR_READ_ONLY;
> + break;
> + case ENOTDIR:
> + fmt = QERR_ENOTDIR;
> + break;
> + case EFBIG:
> + fmt = QERR_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/osdep-qerror.h b/osdep-qerror.h
> new file mode 100644
> index 0000000..7320f4a
> --- /dev/null
> +++ b/osdep-qerror.h
> @@ -0,0 +1,8 @@
> +#ifndef OSDEP_QERROR_H
> +#define OSDEP_QERROR_H
> +
> +#include "error.h"
> +
> +void qemu_fopen_err(Error **errp, const char *file_name);
> +
> +#endif
> diff --git a/osdep.c b/osdep.c
> index 3e6bada..efdd21c 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -241,4 +241,3 @@ ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags)
>
> return total;
> }
> -
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/5] add qemu_fopen_err
2012-03-23 14:22 ` Luiz Capitulino
@ 2012-03-23 19:55 ` Alon Levy
0 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-23 19:55 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
On Fri, Mar 23, 2012 at 11:22:48AM -0300, Luiz Capitulino wrote:
> On Sun, 18 Mar 2012 19:29:10 +0100
> Alon Levy <alevy@redhat.com> wrote:
>
> > This adds a helper to conveniently set the correct error based on the
> > errno after a failed fopen.
> >
> > The added function is placed in it's own c file to allow libcacard to
> > not develop dependencies on everything that qerror will bring in, which
> > includes monitor and half of qemu. I tried to make it as less ugly as I
> > could, by naming an osdep-no-qerror-obj-y and having that included in
> > osdep-obj-y, and using only the former for libcacard.
>
> I'm not sure I like this, how will libcacard report errors then?
libcacard is a standalone library, it reports errors via it's existing
interfaces. Changing it to use qerror is possibly a good idea but not
part of this patchset.
>
> >
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> > Makefile.objs | 8 +++++---
> > libcacard/Makefile | 2 +-
> > osdep-qerror.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > osdep-qerror.h | 8 ++++++++
> > osdep.c | 1 -
> > 5 files changed, 66 insertions(+), 5 deletions(-)
> > create mode 100644 osdep-qerror.c
> > create mode 100644 osdep-qerror.h
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 226b01d..fb5a73a 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -20,9 +20,11 @@ universal-obj-y += $(qom-obj-y)
> >
> > #######################################################################
> > # oslib-obj-y is code depending on the OS (win32 vs posix)
> > -oslib-obj-y = osdep.o
> > -oslib-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o
> > -oslib-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
> > +oslib-no-qerror-obj-y = osdep.o
> > +oslib-no-qerror-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o
> > +oslib-no-qerror-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
> > +oslib-obj-y = $(oslib-no-qerror-obj-y)
> > +oslib-obj-y += osdep-qerror.o
> >
> > #######################################################################
> > # coroutines
> > diff --git a/libcacard/Makefile b/libcacard/Makefile
> > index c6a896a..83f483f 100644
> > --- a/libcacard/Makefile
> > +++ b/libcacard/Makefile
> > @@ -8,7 +8,7 @@ libcacard_includedir=$(includedir)/cacard
> > $(call set-vpath, $(SRC_PATH):$(libcacard_srcpath))
> >
> > # objects linked against normal qemu binaries, not compiled with libtool
> > -QEMU_OBJS=$(addprefix ../,$(oslib-obj-y) qemu-timer-common.o $(trace-obj-y))
> > +QEMU_OBJS=$(addprefix ../,$(oslib-no-qerror-obj-y) qemu-timer-common.o $(trace-obj-y))
> >
> > # objects linked into a shared library, built with libtool with -fPIC if required
> > QEMU_OBJS_LIB=$(addsuffix .lo,$(basename $(QEMU_OBJS)))
> > diff --git a/osdep-qerror.c b/osdep-qerror.c
> > new file mode 100644
> > index 0000000..6dac984
> > --- /dev/null
> > +++ b/osdep-qerror.c
> > @@ -0,0 +1,52 @@
> > +#include "qerror.h"
> > +
> > +#include "osdep-qerror.h"
> > +
> > +/*
> > + * Helper to set an Error after a failed fopen.
> > + *
> > + * Uses errno so it must not be changed by another intermediate call.
> > + */
> > +void qemu_fopen_err(Error **errp, const char *file_name)
> > +{
> > + const char *fmt = NULL;
>
> Where's the fopen() call and the mode argument?
>
> > +
> > + switch (errno) {
> > + case EACCES:
> > + fmt = QERR_EACCES;
> > + break;
> > + case EINTR:
> > + fmt = QERR_EINTR;
> > + break;
> > + case EEXIST:
> > + fmt = QERR_EEXIST;
> > + break;
> > + case EMFILE:
> > + fmt = QERR_OPEN_FILE_EMFILE;
> > + break;
> > + case ENOSPC:
> > + fmt = QERR_ENOSPC;
> > + break;
> > + case EPERM:
> > + fmt = QERR_EPERM;
> > + break;
> > + case EROFS:
> > + fmt = QERR_READ_ONLY;
> > + break;
> > + case ENOTDIR:
> > + fmt = QERR_ENOTDIR;
> > + break;
> > + case EFBIG:
> > + fmt = QERR_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/osdep-qerror.h b/osdep-qerror.h
> > new file mode 100644
> > index 0000000..7320f4a
> > --- /dev/null
> > +++ b/osdep-qerror.h
> > @@ -0,0 +1,8 @@
> > +#ifndef OSDEP_QERROR_H
> > +#define OSDEP_QERROR_H
> > +
> > +#include "error.h"
> > +
> > +void qemu_fopen_err(Error **errp, const char *file_name);
> > +
> > +#endif
> > diff --git a/osdep.c b/osdep.c
> > index 3e6bada..efdd21c 100644
> > --- a/osdep.c
> > +++ b/osdep.c
> > @@ -241,4 +241,3 @@ ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags)
> >
> > return total;
> > }
> > -
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v3 3/5] vga_hw_screen_dump: add Error** param
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 0/5] screendump qapi convertion Alon Levy
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure Alon Levy
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 2/5] add qemu_fopen_err Alon Levy
@ 2012-03-18 18:29 ` Alon Levy
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 4/5] qapi: convert screendump Alon Levy
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 5/5] vga: ppm_save(): Return error on failure Alon Levy
4 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-18 18:29 UTC (permalink / raw)
To: qemu-devel, Luiz Capitulino
To later use for qapi implementation of screendump.
Signed-off-by: Alon Levy <alevy@redhat.com>
---
console.c | 5 +++--
console.h | 6 ++++--
hw/blizzard.c | 3 ++-
hw/g364fb.c | 4 +++-
hw/omap_lcdc.c | 4 +++-
hw/qxl.c | 6 ++++--
hw/tcx.c | 13 +++++++++----
hw/vga.c | 7 +++++--
hw/vmware_vga.c | 6 ++++--
monitor.c | 2 +-
10 files changed, 38 insertions(+), 18 deletions(-)
diff --git a/console.c b/console.c
index 6a463f5..d3fccf3 100644
--- a/console.c
+++ b/console.c
@@ -24,6 +24,7 @@
#include "qemu-common.h"
#include "console.h"
#include "qemu-timer.h"
+#include "error.h"
//#define DEBUG_CONSOLE
#define DEFAULT_BACKSCROLL 512
@@ -173,7 +174,7 @@ void vga_hw_invalidate(void)
active_console->hw_invalidate(active_console->hw);
}
-void vga_hw_screen_dump(const char *filename)
+void vga_hw_screen_dump(const char *filename, Error **errp)
{
TextConsole *previous_active_console;
bool cswitch;
@@ -187,7 +188,7 @@ void vga_hw_screen_dump(const char *filename)
console_select(0);
}
if (consoles[0] && consoles[0]->hw_screen_dump) {
- consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
+ consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, errp);
} else {
error_report("screen dump not implemented");
}
diff --git a/console.h b/console.h
index 4334db5..caf13f5 100644
--- a/console.h
+++ b/console.h
@@ -6,6 +6,7 @@
#include "notify.h"
#include "monitor.h"
#include "trace.h"
+#include "error.h"
/* keyboard/mouse support */
@@ -343,7 +344,8 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
typedef void (*vga_hw_update_ptr)(void *);
typedef void (*vga_hw_invalidate_ptr)(void *);
-typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
+typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch,
+ Error **errp);
typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
DisplayState *graphic_console_init(vga_hw_update_ptr update,
@@ -354,7 +356,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
void vga_hw_update(void);
void vga_hw_invalidate(void);
-void vga_hw_screen_dump(const char *filename);
+void vga_hw_screen_dump(const char *filename, Error **errp);
void vga_hw_text_update(console_ch_t *chardata);
int is_graphic_console(void);
diff --git a/hw/blizzard.c b/hw/blizzard.c
index c7d844d..76df78c 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -23,6 +23,7 @@
#include "devices.h"
#include "vga_int.h"
#include "pixel_ops.h"
+#include "error.h"
typedef void (*blizzard_fn_t)(uint8_t *, const uint8_t *, unsigned int);
@@ -933,7 +934,7 @@ static void blizzard_update_display(void *opaque)
}
static void blizzard_screen_dump(void *opaque, const char *filename,
- bool cswitch)
+ bool cswitch, Error **errp)
{
BlizzardState *s = (BlizzardState *) opaque;
diff --git a/hw/g364fb.c b/hw/g364fb.c
index 3a0b68f..7774d05 100644
--- a/hw/g364fb.c
+++ b/hw/g364fb.c
@@ -22,6 +22,7 @@
#include "pixel_ops.h"
#include "trace.h"
#include "sysbus.h"
+#include "error.h"
typedef struct G364State {
/* hardware */
@@ -289,7 +290,8 @@ static void g364fb_reset(G364State *s)
g364fb_invalidate_display(s);
}
-static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
G364State *s = opaque;
int y, x;
diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
index f172093..aec7210 100644
--- a/hw/omap_lcdc.c
+++ b/hw/omap_lcdc.c
@@ -20,6 +20,7 @@
#include "console.h"
#include "omap.h"
#include "framebuffer.h"
+#include "error.h"
struct omap_lcd_panel_s {
MemoryRegion *sysmem;
@@ -264,7 +265,8 @@ static int ppm_save(const char *filename, uint8_t *data,
return 0;
}
-static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void omap_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
struct omap_lcd_panel_s *omap_lcd = opaque;
if (cswitch) {
diff --git a/hw/qxl.c b/hw/qxl.c
index e17b0e3..27f27f5 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -23,6 +23,7 @@
#include "qemu-queue.h"
#include "monitor.h"
#include "sysemu.h"
+#include "error.h"
#include "qxl.h"
@@ -1492,7 +1493,8 @@ static void qxl_hw_invalidate(void *opaque)
vga->invalidate(vga);
}
-static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
PCIQXLDevice *qxl = opaque;
VGACommonState *vga = &qxl->vga;
@@ -1504,7 +1506,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
ppm_save(filename, qxl->ssd.ds->surface);
break;
case QXL_MODE_VGA:
- vga->screen_dump(vga, filename, cswitch);
+ vga->screen_dump(vga, filename, cswitch, errp);
break;
default:
break;
diff --git a/hw/tcx.c b/hw/tcx.c
index ac7dcb4..f342a5d 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -26,6 +26,7 @@
#include "pixel_ops.h"
#include "sysbus.h"
#include "qdev-addr.h"
+#include "error.h"
#define MAXX 1024
#define MAXY 768
@@ -56,8 +57,10 @@ typedef struct TCXState {
uint8_t dac_index, dac_state;
} TCXState;
-static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch);
-static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch);
+static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error *errp);
+static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error *errp);
static void tcx_set_dirty(TCXState *s)
{
@@ -574,7 +577,8 @@ static int tcx_init1(SysBusDevice *dev)
return 0;
}
-static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error *errp)
{
TCXState *s = opaque;
FILE *f;
@@ -601,7 +605,8 @@ static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
return;
}
-static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error *errp)
{
TCXState *s = opaque;
FILE *f;
diff --git a/hw/vga.c b/hw/vga.c
index 6dc98f6..79c5c38 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -31,6 +31,7 @@
#include "qemu-timer.h"
#include "xen.h"
#include "trace.h"
+#include "error.h"
//#define DEBUG_VGA
//#define DEBUG_VGA_MEM
@@ -163,7 +164,8 @@ static uint32_t expand4[256];
static uint16_t expand2[256];
static uint8_t expand4to8[16];
-static void vga_screen_dump(void *opaque, const char *filename, bool cswitch);
+static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp);
static void vga_update_memory_access(VGACommonState *s)
{
@@ -2409,7 +2411,8 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
/* save the vga display in a PPM image even if no display is
available */
-static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
VGACommonState *s = opaque;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 142d9f4..6868778 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -26,6 +26,7 @@
#include "console.h"
#include "pci.h"
#include "vmware_vga.h"
+#include "error.h"
#undef VERBOSE
#define HW_RECT_ACCEL
@@ -1003,11 +1004,12 @@ static void vmsvga_invalidate_display(void *opaque)
/* save the vga display in a PPM image even if no display is
available */
-static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
+ Error **errp)
{
struct vmsvga_state_s *s = opaque;
if (!s->enable) {
- s->vga.screen_dump(&s->vga, filename, cswitch);
+ s->vga.screen_dump(&s->vga, filename, cswitch, errp);
return;
}
diff --git a/monitor.c b/monitor.c
index d57e7bf..2156fcf 100644
--- a/monitor.c
+++ b/monitor.c
@@ -895,7 +895,7 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
- vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
+ vga_hw_screen_dump(qdict_get_str(qdict, "filename"), NULL);
return 0;
}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v3 4/5] qapi: convert screendump
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 0/5] screendump qapi convertion Alon Levy
` (2 preceding siblings ...)
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 3/5] vga_hw_screen_dump: add Error** param Alon Levy
@ 2012-03-18 18:29 ` Alon Levy
2012-03-23 14:25 ` Luiz Capitulino
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 5/5] vga: ppm_save(): Return error on failure Alon Levy
4 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-18 18:29 UTC (permalink / raw)
To: qemu-devel, Luiz Capitulino
The documenting comment contains the long list of possible errors from
qemu_fopen_err, this could probably be put somewhere else in the file
once more of the api uses those common error classes.
Signed-off-by: Alon Levy <alevy@redhat.com>
---
hmp-commands.hx | 3 +--
hmp.c | 8 ++++++++
hmp.h | 1 +
monitor.c | 6 ------
qapi-schema.json | 24 ++++++++++++++++++++++++
qmp-commands.hx | 5 +----
qmp.c | 5 +++++
7 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 6980214..d26421a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -194,8 +194,7 @@ ETEXI
.args_type = "filename:F",
.params = "filename",
.help = "save screen into PPM image 'filename'",
- .user_print = monitor_user_noop,
- .mhandler.cmd_new = do_screen_dump,
+ .mhandler.cmd = hmp_screendump,
},
STEXI
diff --git a/hmp.c b/hmp.c
index 290c43d..42dc79a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -860,3 +860,11 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &error);
}
+
+void hmp_screendump(Monitor *mon, const QDict *qdict)
+{
+ Error *err = NULL;
+
+ qmp_screendump(qdict_get_str(qdict, "filename"), &err);
+ hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 5409464..25d123f 100644
--- a/hmp.h
+++ b/hmp.h
@@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
void hmp_block_stream(Monitor *mon, const QDict *qdict);
void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
+void hmp_screendump(Monitor *mon, const QDict *qdict);
#endif
diff --git a/monitor.c b/monitor.c
index 2156fcf..34d7617 100644
--- a/monitor.c
+++ b/monitor.c
@@ -893,12 +893,6 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
return -1;
}
-static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
-{
- vga_hw_screen_dump(qdict_get_str(qdict, "filename"), NULL);
- return 0;
-}
-
static void do_logfile(Monitor *mon, const QDict *qdict)
{
cpu_set_log_filename(qdict_get_str(qdict, "filename"));
diff --git a/qapi-schema.json b/qapi-schema.json
index 04fa84f..b9baba9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1663,3 +1663,27 @@
{ 'command': 'qom-list-types',
'data': { '*implements': 'str', '*abstract': 'bool' },
'returns': [ 'ObjectTypeInfo' ] }
+
+##
+# @screendump:
+#
+# Write a PPM of the VGA screen to a file.
+#
+# @filename: the name of a new PPM file to create to store the image
+#
+# Returns: Nothing on success
+# If @cpu is not a valid VCPU, InvalidParameterValue
+# If @filename cannot be opened, OpenFileFailed or one of the more
+# specific errors:
+# EINTR - Interruped during operation
+# EACCES - Cannot access file
+# OpenFileEMFILE - Maximum open file descriptors reached
+# ENOSPC - No space on device
+# EPERM - No permission
+# READ_ONLY - File system is read only
+# ENOTDIR - Path to file contains a non directory element
+#
+##
+# Since: 1.1
+##
+{ 'command': 'screendump', 'data': {'filename': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index dfe8a5b..5fe57fd 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -146,10 +146,7 @@ EQMP
{
.name = "screendump",
.args_type = "filename:F",
- .params = "filename",
- .help = "save screen into PPM image 'filename'",
- .user_print = monitor_user_noop,
- .mhandler.cmd_new = do_screen_dump,
+ .mhandler.cmd_new = qmp_marshal_input_screendump,
},
SQMP
diff --git a/qmp.c b/qmp.c
index a182b51..086cec8 100644
--- a/qmp.c
+++ b/qmp.c
@@ -415,3 +415,8 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
return ret;
}
+
+void qmp_screendump(const char *filename, Error **errp)
+{
+ vga_hw_screen_dump(filename, errp);
+}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/5] qapi: convert screendump
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 4/5] qapi: convert screendump Alon Levy
@ 2012-03-23 14:25 ` Luiz Capitulino
0 siblings, 0 replies; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-23 14:25 UTC (permalink / raw)
To: Alon Levy; +Cc: qemu-devel
On Sun, 18 Mar 2012 19:29:12 +0100
Alon Levy <alevy@redhat.com> wrote:
> The documenting comment contains the long list of possible errors from
> qemu_fopen_err, this could probably be put somewhere else in the file
> once more of the api uses those common error classes.
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
> hmp-commands.hx | 3 +--
> hmp.c | 8 ++++++++
> hmp.h | 1 +
> monitor.c | 6 ------
> qapi-schema.json | 24 ++++++++++++++++++++++++
> qmp-commands.hx | 5 +----
> qmp.c | 5 +++++
> 7 files changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 6980214..d26421a 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -194,8 +194,7 @@ ETEXI
> .args_type = "filename:F",
> .params = "filename",
> .help = "save screen into PPM image 'filename'",
> - .user_print = monitor_user_noop,
> - .mhandler.cmd_new = do_screen_dump,
> + .mhandler.cmd = hmp_screendump,
> },
>
> STEXI
> diff --git a/hmp.c b/hmp.c
> index 290c43d..42dc79a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -860,3 +860,11 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
>
> hmp_handle_error(mon, &error);
> }
> +
> +void hmp_screendump(Monitor *mon, const QDict *qdict)
> +{
> + Error *err = NULL;
> +
> + qmp_screendump(qdict_get_str(qdict, "filename"), &err);
> + hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 5409464..25d123f 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -59,5 +59,6 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
> void hmp_block_stream(Monitor *mon, const QDict *qdict);
> void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
> void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> +void hmp_screendump(Monitor *mon, const QDict *qdict);
>
> #endif
> diff --git a/monitor.c b/monitor.c
> index 2156fcf..34d7617 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -893,12 +893,6 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
> return -1;
> }
>
> -static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> -{
> - vga_hw_screen_dump(qdict_get_str(qdict, "filename"), NULL);
> - return 0;
> -}
> -
> static void do_logfile(Monitor *mon, const QDict *qdict)
> {
> cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 04fa84f..b9baba9 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1663,3 +1663,27 @@
> { 'command': 'qom-list-types',
> 'data': { '*implements': 'str', '*abstract': 'bool' },
> 'returns': [ 'ObjectTypeInfo' ] }
> +
> +##
> +# @screendump:
> +#
> +# Write a PPM of the VGA screen to a file.
> +#
> +# @filename: the name of a new PPM file to create to store the image
> +#
> +# Returns: Nothing on success
> +# If @cpu is not a valid VCPU, InvalidParameterValue
> +# If @filename cannot be opened, OpenFileFailed or one of the more
> +# specific errors:
> +# EINTR - Interruped during operation
> +# EACCES - Cannot access file
> +# OpenFileEMFILE - Maximum open file descriptors reached
> +# ENOSPC - No space on device
> +# EPERM - No permission
> +# READ_ONLY - File system is read only
> +# ENOTDIR - Path to file contains a non directory element
We write the errors a bit different, please take a look at other commands.
> +#
> +##
> +# Since: 1.1
> +##
> +{ 'command': 'screendump', 'data': {'filename': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index dfe8a5b..5fe57fd 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -146,10 +146,7 @@ EQMP
> {
> .name = "screendump",
> .args_type = "filename:F",
> - .params = "filename",
> - .help = "save screen into PPM image 'filename'",
> - .user_print = monitor_user_noop,
> - .mhandler.cmd_new = do_screen_dump,
> + .mhandler.cmd_new = qmp_marshal_input_screendump,
> },
>
> SQMP
> diff --git a/qmp.c b/qmp.c
> index a182b51..086cec8 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -415,3 +415,8 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>
> return ret;
> }
> +
> +void qmp_screendump(const char *filename, Error **errp)
> +{
> + vga_hw_screen_dump(filename, errp);
> +}
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v3 5/5] vga: ppm_save(): Return error on failure
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 0/5] screendump qapi convertion Alon Levy
` (3 preceding siblings ...)
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 4/5] qapi: convert screendump Alon Levy
@ 2012-03-18 18:29 ` Alon Levy
2012-03-23 14:27 ` Luiz Capitulino
4 siblings, 1 reply; 31+ messages in thread
From: Alon Levy @ 2012-03-18 18:29 UTC (permalink / raw)
To: qemu-devel, Luiz Capitulino
From: Luiz Capitulino <lcapitulino@redhat.com>
This makes all devices using ppm_save() return an error appropriately
when the screendump command fails.
Based on a code by Anthony Liguori.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Alon Levy <alevy@redhat.com>
---
hw/blizzard.c | 2 +-
hw/qxl.c | 2 +-
hw/vga.c | 8 +++++---
hw/vga_int.h | 3 ++-
hw/vmware_vga.c | 2 +-
5 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/hw/blizzard.c b/hw/blizzard.c
index 76df78c..29e5ae6 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -942,7 +942,7 @@ static void blizzard_screen_dump(void *opaque, const char *filename,
blizzard_update_display(opaque);
}
if (s && ds_get_data(s->state))
- ppm_save(filename, s->state->surface);
+ ppm_save(filename, s->state->surface, errp);
}
#define DEPTH 8
diff --git a/hw/qxl.c b/hw/qxl.c
index 27f27f5..aa68612 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1503,7 +1503,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
case QXL_MODE_COMPAT:
case QXL_MODE_NATIVE:
qxl_render_update(qxl);
- ppm_save(filename, qxl->ssd.ds->surface);
+ ppm_save(filename, qxl->ssd.ds->surface, errp);
break;
case QXL_MODE_VGA:
vga->screen_dump(vga, filename, cswitch, errp);
diff --git a/hw/vga.c b/hw/vga.c
index 79c5c38..80e6dca 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2365,7 +2365,7 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
/********************************************************/
/* vga screen dump */
-int ppm_save(const char *filename, struct DisplaySurface *ds)
+int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp)
{
FILE *f;
uint8_t *d, *d1;
@@ -2377,8 +2377,10 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
trace_ppm_save(filename, ds);
f = fopen(filename, "wb");
- if (!f)
+ if (!f) {
+ error_set_file_open_failed(errp, filename, errno);
return -1;
+ }
fprintf(f, "P6\n%d %d\n%d\n",
ds->width, ds->height, 255);
linebuf = g_malloc(ds->width * 3);
@@ -2420,5 +2422,5 @@ static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
vga_invalidate_display(s);
vga_hw_update();
}
- ppm_save(filename, s->ds->surface);
+ ppm_save(filename, s->ds->surface, errp);
}
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 7685b2b..63078ba 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -24,6 +24,7 @@
#include <hw/hw.h>
#include "memory.h"
+#include "error.h"
#define ST01_V_RETRACE 0x08
#define ST01_DISP_ENABLE 0x01
@@ -200,7 +201,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val);
uint32_t vga_mem_readb(VGACommonState *s, target_phys_addr_t addr);
void vga_mem_writeb(VGACommonState *s, target_phys_addr_t addr, uint32_t val);
void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
-int ppm_save(const char *filename, struct DisplaySurface *ds);
+int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp);
int vga_ioport_invalid(VGACommonState *s, uint32_t addr);
void vga_init_vbe(VGACommonState *s, MemoryRegion *address_space);
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 6868778..0769652 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1016,7 +1016,7 @@ static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
if (s->depth == 32) {
DisplaySurface *ds = qemu_create_displaysurface_from(s->width,
s->height, 32, ds_get_linesize(s->vga.ds), s->vga.vram_ptr);
- ppm_save(filename, ds);
+ ppm_save(filename, ds, errp);
g_free(ds);
}
}
--
1.7.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] vga: ppm_save(): Return error on failure
2012-03-18 18:29 ` [Qemu-devel] [PATCH v3 5/5] vga: ppm_save(): Return error on failure Alon Levy
@ 2012-03-23 14:27 ` Luiz Capitulino
2012-03-23 19:54 ` Alon Levy
0 siblings, 1 reply; 31+ messages in thread
From: Luiz Capitulino @ 2012-03-23 14:27 UTC (permalink / raw)
To: Alon Levy; +Cc: qemu-devel
On Sun, 18 Mar 2012 19:29:13 +0100
Alon Levy <alevy@redhat.com> wrote:
> From: Luiz Capitulino <lcapitulino@redhat.com>
>
> This makes all devices using ppm_save() return an error appropriately
> when the screendump command fails.
>
> Based on a code by Anthony Liguori.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
> hw/blizzard.c | 2 +-
> hw/qxl.c | 2 +-
> hw/vga.c | 8 +++++---
> hw/vga_int.h | 3 ++-
> hw/vmware_vga.c | 2 +-
> 5 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/blizzard.c b/hw/blizzard.c
> index 76df78c..29e5ae6 100644
> --- a/hw/blizzard.c
> +++ b/hw/blizzard.c
> @@ -942,7 +942,7 @@ static void blizzard_screen_dump(void *opaque, const char *filename,
> blizzard_update_display(opaque);
> }
> if (s && ds_get_data(s->state))
> - ppm_save(filename, s->state->surface);
> + ppm_save(filename, s->state->surface, errp);
> }
>
> #define DEPTH 8
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 27f27f5..aa68612 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1503,7 +1503,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
> case QXL_MODE_COMPAT:
> case QXL_MODE_NATIVE:
> qxl_render_update(qxl);
> - ppm_save(filename, qxl->ssd.ds->surface);
> + ppm_save(filename, qxl->ssd.ds->surface, errp);
> break;
> case QXL_MODE_VGA:
> vga->screen_dump(vga, filename, cswitch, errp);
> diff --git a/hw/vga.c b/hw/vga.c
> index 79c5c38..80e6dca 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2365,7 +2365,7 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
> /********************************************************/
> /* vga screen dump */
>
> -int ppm_save(const char *filename, struct DisplaySurface *ds)
> +int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp)
> {
> FILE *f;
> uint8_t *d, *d1;
> @@ -2377,8 +2377,10 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>
> trace_ppm_save(filename, ds);
> f = fopen(filename, "wb");
What I suggested to you was calling qemu_fopen_err() instead of fopen().
> - if (!f)
> + if (!f) {
> + error_set_file_open_failed(errp, filename, errno);
Does this even compile?
Also note that there are device models that don't use ppm_save(), they have to
be converted too.
> return -1;
> + }
> fprintf(f, "P6\n%d %d\n%d\n",
> ds->width, ds->height, 255);
> linebuf = g_malloc(ds->width * 3);
> @@ -2420,5 +2422,5 @@ static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
> vga_invalidate_display(s);
> vga_hw_update();
> }
> - ppm_save(filename, s->ds->surface);
> + ppm_save(filename, s->ds->surface, errp);
> }
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 7685b2b..63078ba 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -24,6 +24,7 @@
>
> #include <hw/hw.h>
> #include "memory.h"
> +#include "error.h"
>
> #define ST01_V_RETRACE 0x08
> #define ST01_DISP_ENABLE 0x01
> @@ -200,7 +201,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val);
> uint32_t vga_mem_readb(VGACommonState *s, target_phys_addr_t addr);
> void vga_mem_writeb(VGACommonState *s, target_phys_addr_t addr, uint32_t val);
> void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
> -int ppm_save(const char *filename, struct DisplaySurface *ds);
> +int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp);
>
> int vga_ioport_invalid(VGACommonState *s, uint32_t addr);
> void vga_init_vbe(VGACommonState *s, MemoryRegion *address_space);
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 6868778..0769652 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1016,7 +1016,7 @@ static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
> if (s->depth == 32) {
> DisplaySurface *ds = qemu_create_displaysurface_from(s->width,
> s->height, 32, ds_get_linesize(s->vga.ds), s->vga.vram_ptr);
> - ppm_save(filename, ds);
> + ppm_save(filename, ds, errp);
> g_free(ds);
> }
> }
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] vga: ppm_save(): Return error on failure
2012-03-23 14:27 ` Luiz Capitulino
@ 2012-03-23 19:54 ` Alon Levy
0 siblings, 0 replies; 31+ messages in thread
From: Alon Levy @ 2012-03-23 19:54 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
On Fri, Mar 23, 2012 at 11:27:02AM -0300, Luiz Capitulino wrote:
> On Sun, 18 Mar 2012 19:29:13 +0100
> Alon Levy <alevy@redhat.com> wrote:
>
> > From: Luiz Capitulino <lcapitulino@redhat.com>
> >
> > This makes all devices using ppm_save() return an error appropriately
> > when the screendump command fails.
> >
> > Based on a code by Anthony Liguori.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> > hw/blizzard.c | 2 +-
> > hw/qxl.c | 2 +-
> > hw/vga.c | 8 +++++---
> > hw/vga_int.h | 3 ++-
> > hw/vmware_vga.c | 2 +-
> > 5 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/blizzard.c b/hw/blizzard.c
> > index 76df78c..29e5ae6 100644
> > --- a/hw/blizzard.c
> > +++ b/hw/blizzard.c
> > @@ -942,7 +942,7 @@ static void blizzard_screen_dump(void *opaque, const char *filename,
> > blizzard_update_display(opaque);
> > }
> > if (s && ds_get_data(s->state))
> > - ppm_save(filename, s->state->surface);
> > + ppm_save(filename, s->state->surface, errp);
> > }
> >
> > #define DEPTH 8
> > diff --git a/hw/qxl.c b/hw/qxl.c
> > index 27f27f5..aa68612 100644
> > --- a/hw/qxl.c
> > +++ b/hw/qxl.c
> > @@ -1503,7 +1503,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
> > case QXL_MODE_COMPAT:
> > case QXL_MODE_NATIVE:
> > qxl_render_update(qxl);
> > - ppm_save(filename, qxl->ssd.ds->surface);
> > + ppm_save(filename, qxl->ssd.ds->surface, errp);
> > break;
> > case QXL_MODE_VGA:
> > vga->screen_dump(vga, filename, cswitch, errp);
> > diff --git a/hw/vga.c b/hw/vga.c
> > index 79c5c38..80e6dca 100644
> > --- a/hw/vga.c
> > +++ b/hw/vga.c
> > @@ -2365,7 +2365,7 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
> > /********************************************************/
> > /* vga screen dump */
> >
> > -int ppm_save(const char *filename, struct DisplaySurface *ds)
> > +int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp)
> > {
> > FILE *f;
> > uint8_t *d, *d1;
> > @@ -2377,8 +2377,10 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
> >
> > trace_ppm_save(filename, ds);
> > f = fopen(filename, "wb");
>
> What I suggested to you was calling qemu_fopen_err() instead of fopen().
Oh, I didn't understand it that way. Makes more sense, will do.
>
> > - if (!f)
> > + if (!f) {
> > + error_set_file_open_failed(errp, filename, errno);
>
> Does this even compile?
I don't recall. Moot now.
>
> Also note that there are device models that don't use ppm_save(), they have to
> be converted too.
>
Will look at them.
> > return -1;
> > + }
> > fprintf(f, "P6\n%d %d\n%d\n",
> > ds->width, ds->height, 255);
> > linebuf = g_malloc(ds->width * 3);
> > @@ -2420,5 +2422,5 @@ static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
> > vga_invalidate_display(s);
> > vga_hw_update();
> > }
> > - ppm_save(filename, s->ds->surface);
> > + ppm_save(filename, s->ds->surface, errp);
> > }
> > diff --git a/hw/vga_int.h b/hw/vga_int.h
> > index 7685b2b..63078ba 100644
> > --- a/hw/vga_int.h
> > +++ b/hw/vga_int.h
> > @@ -24,6 +24,7 @@
> >
> > #include <hw/hw.h>
> > #include "memory.h"
> > +#include "error.h"
> >
> > #define ST01_V_RETRACE 0x08
> > #define ST01_DISP_ENABLE 0x01
> > @@ -200,7 +201,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val);
> > uint32_t vga_mem_readb(VGACommonState *s, target_phys_addr_t addr);
> > void vga_mem_writeb(VGACommonState *s, target_phys_addr_t addr, uint32_t val);
> > void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
> > -int ppm_save(const char *filename, struct DisplaySurface *ds);
> > +int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp);
> >
> > int vga_ioport_invalid(VGACommonState *s, uint32_t addr);
> > void vga_init_vbe(VGACommonState *s, MemoryRegion *address_space);
> > diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> > index 6868778..0769652 100644
> > --- a/hw/vmware_vga.c
> > +++ b/hw/vmware_vga.c
> > @@ -1016,7 +1016,7 @@ static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
> > if (s->depth == 32) {
> > DisplaySurface *ds = qemu_create_displaysurface_from(s->width,
> > s->height, 32, ds_get_linesize(s->vga.ds), s->vga.vram_ptr);
> > - ppm_save(filename, ds);
> > + ppm_save(filename, ds, errp);
> > g_free(ds);
> > }
> > }
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread