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()?  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)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org