* [PATCH] block: Remove trailing newline in format used by error_report API
@ 2020-02-28 12:36 Philippe Mathieu-Daudé
2020-02-28 13:16 ` Liam Merwick
2020-02-28 17:32 ` Markus Armbruster
0 siblings, 2 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-28 12:36 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Kevin Wolf, Philippe Mathieu-Daudé,
qemu-block, Max Reitz
The error_report API doesn't want trailing newline characters.
Remove it, to avoid and error when moving the code around:
ERROR: Error messages should not contain newlines
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 1bdb9c679d..e466d15914 100644
--- a/block.c
+++ b/block.c
@@ -5994,7 +5994,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
/* Couldn't open BS, but we have a size, so it's nonfatal */
warn_reportf_err(local_err,
"Could not verify backing image. "
- "This may become an error in future versions.\n");
+ "This may become an error in future versions.");
local_err = NULL;
} else if (!bs) {
/* Couldn't open bs, do not have size */
--
2.21.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] block: Remove trailing newline in format used by error_report API
2020-02-28 12:36 [PATCH] block: Remove trailing newline in format used by error_report API Philippe Mathieu-Daudé
@ 2020-02-28 13:16 ` Liam Merwick
2020-02-28 17:32 ` Markus Armbruster
1 sibling, 0 replies; 6+ messages in thread
From: Liam Merwick @ 2020-02-28 13:16 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: qemu-trivial, Kevin Wolf, qemu-block, Max Reitz
On 28/02/2020 12:36, Philippe Mathieu-Daudé wrote:
> The error_report API doesn't want trailing newline characters.
> Remove it, to avoid and error when moving the code around:
>
s/and/an/
> ERROR: Error messages should not contain newlines
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
> ---
> block.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 1bdb9c679d..e466d15914 100644
> --- a/block.c
> +++ b/block.c
> @@ -5994,7 +5994,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> /* Couldn't open BS, but we have a size, so it's nonfatal */
> warn_reportf_err(local_err,
> "Could not verify backing image. "
> - "This may become an error in future versions.\n");
> + "This may become an error in future versions.");
> local_err = NULL;
> } else if (!bs) {
> /* Couldn't open bs, do not have size */
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: Remove trailing newline in format used by error_report API
2020-02-28 12:36 [PATCH] block: Remove trailing newline in format used by error_report API Philippe Mathieu-Daudé
2020-02-28 13:16 ` Liam Merwick
@ 2020-02-28 17:32 ` Markus Armbruster
2020-06-05 14:26 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2020-02-28 17:32 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, qemu-trivial, qemu-devel, Max Reitz, John Snow
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> The error_report API doesn't want trailing newline characters.
> Remove it, to avoid and error when moving the code around:
>
> ERROR: Error messages should not contain newlines
Commit 312fd5f2909 has a Coccinelle script. It should be committed and
re-run.
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 1bdb9c679d..e466d15914 100644
> --- a/block.c
> +++ b/block.c
> @@ -5994,7 +5994,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
&local_err);
g_free(full_backing);
if (!bs && size != -1) {
> /* Couldn't open BS, but we have a size, so it's nonfatal */
> warn_reportf_err(local_err,
> "Could not verify backing image. "
> - "This may become an error in future versions.\n");
> + "This may become an error in future versions.");
> local_err = NULL;
> } else if (!bs) {
> /* Couldn't open bs, do not have size */
warn_reportf_err() is a convenience function to error_prepend(),
warn_report() and free @local_err.
When @local_err holds a message like "pants on fire", the code before
the patch prints something like
qemu-system-x86_64: warning: Could not verify backing image. This may become an error in future versions.
pants on fire
The patch "improves" it to
qemu-system-x86_64: warning: Could not verify backing image. This may become an error in future versions.pants on fire
General advice: this misuse of warn_reportf_err() is an excusable
mistake, but when you *test* the error path, you can't *not* see that
the actual message is crap. Test your errors!
Actual improvement:
warn_reportf_err(local_err, "Could not verify backing image: ");
error_printf("This may become an error in future versions.\n");
This should print
qemu-system-x86_64: warning: Could not verify backing image: pants on fire
This may become an error in future versions.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: Remove trailing newline in format used by error_report API
2020-02-28 17:32 ` Markus Armbruster
@ 2020-06-05 14:26 ` Philippe Mathieu-Daudé
2020-06-08 4:45 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-05 14:26 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, qemu-block, qemu-trivial, qemu-devel, Max Reitz, John Snow
On 2/28/20 6:32 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> The error_report API doesn't want trailing newline characters.
>> Remove it, to avoid and error when moving the code around:
>>
>> ERROR: Error messages should not contain newlines
>
> Commit 312fd5f2909 has a Coccinelle script. It should be committed and
> re-run.
>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> block.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 1bdb9c679d..e466d15914 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5994,7 +5994,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
> &local_err);
> g_free(full_backing);
> if (!bs && size != -1) {
>> /* Couldn't open BS, but we have a size, so it's nonfatal */
>> warn_reportf_err(local_err,
>> "Could not verify backing image. "
>> - "This may become an error in future versions.\n");
>> + "This may become an error in future versions.");
>> local_err = NULL;
>> } else if (!bs) {
>> /* Couldn't open bs, do not have size */
>
> warn_reportf_err() is a convenience function to error_prepend(),
> warn_report() and free @local_err.
OK now I see.
Why warn_reportf_err() doesn't take a 'Error **err' instead, to set err
to NULL after freeing *err?
>
> When @local_err holds a message like "pants on fire", the code before
> the patch prints something like
>
> qemu-system-x86_64: warning: Could not verify backing image. This may become an error in future versions.
> pants on fire
>
> The patch "improves" it to
>
> qemu-system-x86_64: warning: Could not verify backing image. This may become an error in future versions.pants on fire
>
> General advice: this misuse of warn_reportf_err() is an excusable
> mistake, but when you *test* the error path, you can't *not* see that
> the actual message is crap. Test your errors!
>
> Actual improvement:
>
> warn_reportf_err(local_err, "Could not verify backing image: ");
> error_printf("This may become an error in future versions.\n");
>
> This should print
>
> qemu-system-x86_64: warning: Could not verify backing image: pants on fire
> This may become an error in future versions.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: Remove trailing newline in format used by error_report API
2020-06-05 14:26 ` Philippe Mathieu-Daudé
@ 2020-06-08 4:45 ` Markus Armbruster
2020-06-08 6:14 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2020-06-08 4:45 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, qemu-trivial, qemu-devel, Max Reitz, John Snow
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
[...]
> Why warn_reportf_err() doesn't take a 'Error **err' instead, to set err
> to NULL after freeing *err?
Why doesn't free() take a void ** argument, to set the pointer to null
after freeing what it points to? Why doesn't close() take an int *
argument?
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: Remove trailing newline in format used by error_report API
2020-06-08 4:45 ` Markus Armbruster
@ 2020-06-08 6:14 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-08 6:14 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, qemu-block, qemu-trivial, qemu-devel, Max Reitz, John Snow
On 6/8/20 6:45 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> On 2/28/20 6:32 PM, Markus Armbruster wrote:
[...]
>>> warn_reportf_err() is a convenience function to error_prepend(),
>>> warn_report() and free @local_err.
> [...]
>> Why warn_reportf_err() doesn't take a 'Error **err' instead, to set err
>> to NULL after freeing *err?
>
> Why doesn't free() take a void ** argument, to set the pointer to null
> after freeing what it points to? Why doesn't close() take an int *
> argument?
=)
Usually I see the code checking an Error* hasn't been set by a callee.
If it has, the caller usually returns.
You explained me warn_reportf_err() consume Error* and free() it.
So regarding the rest of our Error* use, a function calling
warn_reportf_err has to do extra care to set Error* to NULL.
Genuinely looks confuse or dangerous to me...
Note however I was not asking for a change, just asking 'why'
to better understand if there were not a design problem, or
o invalid use of different APIs.
>
> [...]
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-08 6:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-28 12:36 [PATCH] block: Remove trailing newline in format used by error_report API Philippe Mathieu-Daudé
2020-02-28 13:16 ` Liam Merwick
2020-02-28 17:32 ` Markus Armbruster
2020-06-05 14:26 ` Philippe Mathieu-Daudé
2020-06-08 4:45 ` Markus Armbruster
2020-06-08 6:14 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.