[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_repor
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err() |
Date: |
Wed, 11 Feb 2015 11:04:07 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
[Qemu-devel] [PATCH 7/9] vl: Avoid qerror_report_err() outside QMP command handlers, Markus Armbruster, 2015/02/10
[Qemu-devel] [PATCH 5/9] numa: Avoid qerror_report_err() outside QMP command handlers, Markus Armbruster, 2015/02/10
[Qemu-devel] [PATCH 9/9] qemu-char: Avoid qerror_report_err() outside QMP command handlers, Markus Armbruster, 2015/02/10
[Qemu-devel] [PATCH 8/9] qemu-img: Avoid qerror_report_err() outside QMP command handlers, Markus Armbruster, 2015/02/10
[Qemu-devel] [PATCH 4/9] net: Avoid qerror_report_err() outside QMP command handlers, Markus Armbruster, 2015/02/10
Re: [Qemu-devel] [PATCH 0/9] Clean up around error_get_pretty(), qerror_report_err(), Eric Blake, 2015/02/11