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

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

[Wei Jiangang]

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__) */
-- 
1.9.3

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

[Fam Zheng]

On Wed, 06/01 15:08, Wei Jiangang wrote:
> 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);

I'd say it's less readable with a worse alignment to the parameters.

Fam

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

[Wei, Jiangang]

On Wed, 2016-06-01 at 15:23 +0800, Fam Zheng wrote:
> On Wed, 06/01 15:08, Wei Jiangang wrote:
> > 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);
> 
> I'd say it's less readable with a worse alignment to the parameters.
Hi, Fam

Thanks for your comment.
You mean every error_report() followed by file_name is more readable ?
or my codes dosen't follow a good alignment format？

Wei
> 
> Fam
> 
> 

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

[Fam Zheng]

On Thu, 06/02 03:04, Wei, Jiangang wrote:
> On Wed, 2016-06-01 at 15:23 +0800, Fam Zheng wrote:
> > On Wed, 06/01 15:08, Wei Jiangang wrote:
> > > 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);
> > 
> > I'd say it's less readable with a worse alignment to the parameters.
> Hi, Fam
> 
> Thanks for your comment.
> You mean every error_report() followed by file_name is more readable ?
> or my codes dosen't follow a good alignment format？
> 

I'm happy with either way. :)

Fam

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

[Markus Armbruster]

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.

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

[Wei, Jiangang]

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

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

[Fam Zheng]

On Thu, 06/02 10:46, Wei, Jiangang wrote:
> My intention is only to avoid the output of multiple location
> information.

Then I think this should have gone into the commit message.

Fam

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

[Wei, Jiangang]

On Fri, 2016-06-03 at 17:00 +0800, Fam Zheng wrote:
> On Thu, 06/02 10:46, Wei, Jiangang wrote:
> > My intention is only to avoid the output of multiple location
> > information.
> 
> Then I think this should have gone into the commit message.

OK, I'll  make more detailed description in next version.
> 
> Fam
> 
>