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
pgp2yZ6f8IzMQ.pgp
Description: PGP signature
