| Message ID | 1464764901-8773-1-git-send-email-weijg.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 > >
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
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.
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 > >
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
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 > >
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__) */
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(-)