On Mon, Jul 25, 2016 at 05:58:54PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefa...@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 <fullma...@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.
signature.asc
Description: PGP signature
