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

["Wei, Jiangang" <weijg.fnst@cn.fujitsu.com>]

On Thu, 2016-06-02 at 09:57 +0200, Markus Armbruster wrote:
> Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:
> 
> > Use a single error_printf to replace triple error_report.
> >
> > Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
> > ---
> >  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.
I think you gave me a wonderful lesson about how to deal with error
report.
Thank you very much.

My intention is only to avoid the output of multiple location
information.

I'll check it again.

Thanks,
wei
> 
>