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

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> 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.

Either that, or combine the messages like this:

    error_report("failed to create inode for snapshot: %s",
                 error_get_pretty(local_err));

Two error_report() in a row are suspect in general.

Aside: do_sd_create() can't decide what to return on failure, negative
value without meaning, or a negative error number.

>> 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.

I guess that depends on the conventions used by the code.  In QEMU,
error reporting is generally an error information sink, i.e. no detailed
information on the reported error is passed further up.

>> > @@ -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.

I can do that.