Re: [Qemu-devel] [PATCH] qemu-img: fix error messages emitted by img_open()

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

On Mon, Jul 25, 2016 at 05:58:54PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > On Thu, Jul 21, 2016 at 10:41:53AM +0200, Reda Sallahi wrote:
> >> img_open_file() and img_open_opts() were printing error messages with a
> >> duplicate part because of a wrong use of error_reportf_err() (e.g.
> >> qemu-img: Could not open 'foo': Could not open 'foo': No such file or directory)
> >> 
> >> This change uses error_report_err() instead to eliminate the duplicate part.
> >> 
> >> Signed-off-by: Reda Sallahi <fullmanet@gmail.com>
> >> ---
> >>  qemu-img.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/qemu-img.c b/qemu-img.c
> >> index 2e40e1f..dc6652d 100644
> >> --- a/qemu-img.c
> >> +++ b/qemu-img.c
> >> @@ -268,7 +268,7 @@ static BlockBackend *img_open_opts(const char *optstr,
> >>      options = qemu_opts_to_qdict(opts, NULL);
> >>      blk = blk_new_open(NULL, NULL, options, flags, &local_err);
> >>      if (!blk) {
> >> -        error_reportf_err(local_err, "Could not open '%s': ", optstr);
> >> +        error_report_err(local_err);
> >>          return NULL;
> >>      }
> >>      blk_set_enable_write_cache(blk, !writethrough);
> >> @@ -295,7 +295,7 @@ static BlockBackend *img_open_file(const char *filename,
> >>  
> >>      blk = blk_new_open(filename, NULL, options, flags, &local_err);
> >>      if (!blk) {
> >> -        error_reportf_err(local_err, "Could not open '%s': ", filename);
> >> +        error_report(local_err);
> >>          return NULL;
> >>      }
> >>      blk_set_enable_write_cache(blk, !writethrough);
> >
> > The duplication happens in the "Could not open 'foo'" case, but other
> > error cases do not include the filename in the error message.
> >
> > We would lose information in those error cases since the filename is no
> > longer included by qemu-img.c in the error message.
> 
> Could you give an example of such an information loss?

The issue is that while this patch eliminates duplication in:

  qemu-img: Could not open 'foo': Could not open 'foo': No such file or directory

It loses the name from:

  qemu-img: Driver 'bar' is not whitelisted

or any other error message in block.c that doesn't include the filename.

This is probably the reason why qemu-img.c prepends "Could not open
'%s'".

> > I'm not aware of a clean way to distinguish Error objects.  Maybe
> > someone else can suggest one.  Otherwise it may be best to leave the
> > code as it is.
> 
> If you need to distinguish different kinds of errors to conditionally
> rewrite the error message so it makes actual sense, chances are the
> error messages that need the rewriting should be improved instead.
> 
> A more legitimate case is when a caller needs to handle different errors
> differently.  Doesn't occur all that often.
> 
> There are two techniques for callers to distinguish different kinds of
> Errors:
> 
> * ErrorClass, use error_get_class() to retrieve it.  This is actually a
>   remnant of the failed "rich" error object idea.  Almost always
>   ERROR_CLASS_GENERIC_ERROR, so this is unlikely to help.
> 
> * Error code separate from the Error object, e.g. the function returns
>   -errno in addition to an Error object.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]