Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Kevin Wolf]

Am 10.02.2015 um 18:20 hat Eric Blake geschrieben:
> On 02/10/2015 09:34 AM, Markus Armbruster wrote:
> > I've typed error_report("%s", error_get_pretty(ERR)) too many times
> > already, and I've fixed too many instances of qerror_report_err(ERR)
> > to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
> > pattern in a convenience function.
> > 
> > Since it's almost invariably followed by error_free(), stuff that into
> > the convenience function as well.
> > 
> 
> > @@ -2234,8 +2225,7 @@ static int sd_snapshot_create(BlockDriverState *bs, 
> > QEMUSnapshotInfo *sn_info)
> >  
> >      ret = do_sd_create(s, &new_vid, 1, &local_err);
> >      if (ret < 0) {
> > -        error_report("%s", error_get_pretty(local_err));;
> > -        error_free(local_err);
> > +        error_report_err(local_err);
> >          error_report("failed to create inode for snapshot. %s",
> >                       strerror(errno));
> 
> Pre-existing bug, so maybe worth a separate patch.  This looks fishy:
> are we guaranteed that errno is unchanged by error_report_err()?

errno doesn't seem to contain anything meaningful here in the first
place, so I think that line should simply be removed.

> On the
> surface, error_vreport() and friends do NOT try to preserve errno; maybe
> your new function should guarantee that errno is not clobbered? (in
> libvirt, we've explicitly made error-reporting convenience functions
> document that they do not clobber errno, because it is much easier for
> callers to report an error then return an errno value without having to
> save errno locally)

Isn't something going wrong if you report an error and pass it on at the
same time? I always thought that the error reporting should be at the
end of the error handling code.

> > @@ -152,6 +152,12 @@ const char *error_get_pretty(Error *err)
> >      return err->msg;
> >  }
> >  
> > +void error_report_err(Error *err)
> > +{
> > +    error_report("%s", error_get_pretty(err));
> > +    error_free(err);
> > +}
> > +
> 
> If it were me, I'd split this patch in two, one that introduces the new
> function (with no clients), and the other which is a strict Coccinelle
> touchup to use it, so that readers don't have to hunt for the meat of
> the change.

Yes, I thought the same.

Kevin

pgpzTKV1B8kxY.pgp
Description: PGP signature