Re: [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message
On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
Signed-off-by: Fabien Chouteau chout...@adacore.com
---
block.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
Hi Fabien,
Please always CC qemu-devel@nongnu.org. All patches must be on
qemu-devel so that the community can review them - not everyone
subscribes to qemu-trivial.
Thanks,
Stefan
diff --git a/block.c b/block.c
index ba67c0d..3bf8eda 100644
--- a/block.c
+++ b/block.c
@@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
/* GetTempFileName requires that its output buffer (4th param)
have length MAX_PATH or greater. */
assert(size = MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
- GetTempFileName(temp_dir, qem, 0, filename)
-? 0 : -GetLastError());
+if (GetTempPath(MAX_PATH, temp_dir) == 0) {
+fprintf(stderr, GetTempPath() error: %d\n, GetLastError());
+return -GetLastError();
+}
+if (GetTempFileName(temp_dir, qem, 0, filename) == 0) {
+fprintf(stderr, GetTempFileName(%s) error: %d\n, temp_dir,
+GetLastError());
+return -GetLastError();
+}
+return 0;
#else
int fd;
const char *tmpdir;
--
1.7.9.5
Re: [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message
Stefan Hajnoczi stefa...@redhat.com writes:
On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
Signed-off-by: Fabien Chouteau chout...@adacore.com
---
block.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
Hi Fabien,
Please always CC qemu-devel@nongnu.org. All patches must be on
qemu-devel so that the community can review them - not everyone
subscribes to qemu-trivial.
Thanks,
Stefan
diff --git a/block.c b/block.c
index ba67c0d..3bf8eda 100644
--- a/block.c
+++ b/block.c
@@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
/* GetTempFileName requires that its output buffer (4th param)
have length MAX_PATH or greater. */
assert(size = MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
- GetTempFileName(temp_dir, qem, 0, filename)
-? 0 : -GetLastError());
+if (GetTempPath(MAX_PATH, temp_dir) == 0) {
+fprintf(stderr, GetTempPath() error: %d\n, GetLastError());
+return -GetLastError();
+}
+if (GetTempFileName(temp_dir, qem, 0, filename) == 0) {
+fprintf(stderr, GetTempFileName(%s) error: %d\n, temp_dir,
+GetLastError());
+return -GetLastError();
+}
+return 0;
#else
int fd;
const char *tmpdir;
get_tmp_filename() is not supposed to print to stderr, that's the
caller's job.
Re: [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message
On 02/04/2013 11:34 AM, Markus Armbruster wrote:
Stefan Hajnoczi stefa...@redhat.com writes:
On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
Signed-off-by: Fabien Chouteau chout...@adacore.com
---
block.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
Hi Fabien,
Please always CC qemu-devel@nongnu.org. All patches must be on
qemu-devel so that the community can review them - not everyone
subscribes to qemu-trivial.
Thanks,
Stefan
diff --git a/block.c b/block.c
index ba67c0d..3bf8eda 100644
--- a/block.c
+++ b/block.c
@@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
/* GetTempFileName requires that its output buffer (4th param)
have length MAX_PATH or greater. */
assert(size = MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
- GetTempFileName(temp_dir, qem, 0, filename)
-? 0 : -GetLastError());
+if (GetTempPath(MAX_PATH, temp_dir) == 0) {
+fprintf(stderr, GetTempPath() error: %d\n, GetLastError());
+return -GetLastError();
+}
+if (GetTempFileName(temp_dir, qem, 0, filename) == 0) {
+fprintf(stderr, GetTempFileName(%s) error: %d\n, temp_dir,
+GetLastError());
+return -GetLastError();
+}
+return 0;
#else
int fd;
const char *tmpdir;
get_tmp_filename() is not supposed to print to stderr, that's the
caller's job.
Why? The caller doesn't know the difference between Windows/Linux
implementation. And the error handling would have to be duplicated.
It's not the first time I add error output in Windows code. Specially in
block/, when there's an error, the only thing you get is: operation not
permitted. It's not very helpful and you have to dig in the code to
find which function failed.
--
Fabien Chouteau
Re: [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message
Fabien Chouteau chout...@adacore.com writes:
On 02/04/2013 11:34 AM, Markus Armbruster wrote:
Stefan Hajnoczi stefa...@redhat.com writes:
On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
Signed-off-by: Fabien Chouteau chout...@adacore.com
---
block.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
Hi Fabien,
Please always CC qemu-devel@nongnu.org. All patches must be on
qemu-devel so that the community can review them - not everyone
subscribes to qemu-trivial.
Thanks,
Stefan
diff --git a/block.c b/block.c
index ba67c0d..3bf8eda 100644
--- a/block.c
+++ b/block.c
@@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
/* GetTempFileName requires that its output buffer (4th param)
have length MAX_PATH or greater. */
assert(size = MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
- GetTempFileName(temp_dir, qem, 0, filename)
-? 0 : -GetLastError());
+if (GetTempPath(MAX_PATH, temp_dir) == 0) {
+fprintf(stderr, GetTempPath() error: %d\n, GetLastError());
+return -GetLastError();
+}
+if (GetTempFileName(temp_dir, qem, 0, filename) == 0) {
+fprintf(stderr, GetTempFileName(%s) error: %d\n, temp_dir,
+GetLastError());
+return -GetLastError();
+}
+return 0;
#else
int fd;
const char *tmpdir;
get_tmp_filename() is not supposed to print to stderr, that's the
caller's job.
Why? The caller doesn't know the difference between Windows/Linux
implementation. And the error handling would have to be duplicated.
The function's (implied) contract is to return an error code without
printing anything. If you want to change the contract to include
reporting the error, you need to implement it both for both arms of the
#ifdef. You also have to demonstrate that all callers are happy with
the change of contract.
Regardless, printing to stderr is wrong. The function can be called on
behalf of a monitor command, and then the error needs to be printed to
the correct monitor. error_report() can do that for you, and more.
It's not the first time I add error output in Windows code. Specially in
block/, when there's an error, the only thing you get is: operation not
permitted. It's not very helpful and you have to dig in the code to
find which function failed.
Good error reporting is hard. Knowledge about the error and its context
gets lost as you move up the call chain. Knowledge about how to report
errors gets lost as you move down.
Re: [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message
On 02/04/2013 01:24 PM, Markus Armbruster wrote: Fabien Chouteau chout...@adacore.com writes: On 02/04/2013 11:34 AM, Markus Armbruster wrote: Why? The caller doesn't know the difference between Windows/Linux implementation. And the error handling would have to be duplicated. The function's (implied) contract is to return an error code without printing anything. If you want to change the contract to include reporting the error, you need to implement it both for both arms of the #ifdef. You also have to demonstrate that all callers are happy with the change of contract. Regardless, printing to stderr is wrong. The function can be called on behalf of a monitor command, and then the error needs to be printed to the correct monitor. error_report() can do that for you, and more. Alright, so I will use error_report() and do it for both Linux and Windows. It's not the first time I add error output in Windows code. Specially in block/, when there's an error, the only thing you get is: operation not permitted. It's not very helpful and you have to dig in the code to find which function failed. Good error reporting is hard. Knowledge about the error and its context gets lost as you move up the call chain. Knowledge about how to report errors gets lost as you move down. You're right, and in my opinion, no error reporting is the worst case. -- Fabien Chouteau
Re: [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message
Am 04.02.2013 14:33, schrieb Fabien Chouteau: On 02/04/2013 01:24 PM, Markus Armbruster wrote: Good error reporting is hard. Knowledge about the error and its context gets lost as you move up the call chain. Knowledge about how to report errors gets lost as you move down. You're right, and in my opinion, no error reporting is the worst case. The best solution would be to pass an Error **errp argument and use error_setg() on it, as done for visitors and QOM. Then the caller can decide whether to pass NULL and the callee can report detailed errors. Invasive change obviously and thus not suitable for 1.4. Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
