[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qemu-img: fix error messages emitted by img_ope
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH] qemu-img: fix error messages emitted by img_open() |
Date: |
Tue, 26 Jul 2016 14:07:35 +0100 |
User-agent: |
Mutt/1.6.2 (2016-07-01) |
On Mon, Jul 25, 2016 at 05:58:54PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <address@hidden> 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 <address@hidden>
> >> ---
> >> 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.
signature.asc
Description: PGP signature