[PATCH] block: Remove trailing newline in format used by error_report API

[PATCH] block: Remove trailing newline in format used by error_report API

[Philippe Mathieu-Daudé]

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

Re: [PATCH] block: Remove trailing newline in format used by error_report API

[Liam Merwick]

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 */
> 

Re: [PATCH] block: Remove trailing newline in format used by error_report API

[Markus Armbruster]

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.

Re: [PATCH] block: Remove trailing newline in format used by error_report API

[Philippe Mathieu-Daudé]

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.
> 

Re: [PATCH] block: Remove trailing newline in format used by error_report API

[Markus Armbruster]

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?

[...]

Re: [PATCH] block: Remove trailing newline in format used by error_report API

[Philippe Mathieu-Daudé]

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.

> 
> [...]
>