Re: [Qemu-block] [Qemu-devel] [PATCH] block/raw-posix: Fix error_report of mounting message

[Markus Armbruster]

Wei Jiangang <address@hidden> writes:

> Use a single error_printf to replace triple error_report.
>
> Signed-off-by: Wei Jiangang <address@hidden>
> ---
>  block/raw-posix.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index a4f5a1b..141b01a 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -2061,11 +2061,11 @@ static bool setup_cdrom(char *bsd_path, Error **errp)
>  /* Prints directions on mounting and unmounting a device */
>  static void print_unmounting_directions(const char *file_name)
>  {
> -    error_report("If device %s is mounted on the desktop, unmount"
> -                 " it first before using it in QEMU", file_name);
> -    error_report("Command to unmount device: diskutil unmountDisk %s",
> -                 file_name);
> -    error_report("Command to mount device: diskutil mountDisk %s", 
> file_name);
> +    error_printf("If device %s is mounted on the desktop, unmount"
> +        " it first before using it in QEMU\n"
> +        "Command to unmount device: diskutil unmountDisk %s\n"
> +        "Command to mount device: diskutil mountDisk %s\n",
> +        file_name, file_name, file_name);
>  }
>  
>  #endif /* defined(__APPLE__) && defined(__MACH__) */

This is less wrong :)

Beware, I have no suitable machine to actually test this, so the
following is based on code inspection.

print_unmounting_directions() is used by hdev_open().  hdev_open() is a
BlockDriver method bdrv_file_open(), and as such returns errors to its
caller via parameter Error **errp.  The caller may or may not report
errors it gets from hdev_open().

If it reports them, then print_unmounting_directions()'s output
"overtakes" the error message, i.e. the "If device ... is mounted ..."
hint is printed before the error message.  Wrong, it should be printed
after the error message.

If it doesn't report them, the hint gets printed anyway.  More wrong.

In any case, printing each line of the hint with error_report() is also
wrong, because that prefixes each line with program name and location
information.  Your patch fixes this part.

A more complete fix uses error_append_hint(errp, ...) instead of
error_printf().  hdev_open() needs to pass errp to
print_unmounting_directions() for that.  errp must have an error set, or
else error_append_hint() makes no sense.