Philippe Mathieu-Daudé <[email protected]> writes: > On 7/23/20 11:44 AM, Stefan Weil wrote: >> Am 23.07.20 um 11:13 schrieb Philippe Mathieu-Daudé: >> >>> error_propagate_prepend() "behaves like error_prepend()", and >>> error_prepend() uses "formatting @fmt, ... like printf()". >>> error_prepend() checks its format string argument, but >>> error_propagate_prepend() does not. Fix that. >>> >>> This would have catched the invalid format introduced in commit >>> b98e8d1230f: >>> >>> CC hw/sd/milkymist-memcard.o >>> hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’: >>> hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching >>> ‘char *’ argument [-Werror=format=] >>> 284 | error_propagate_prepend(errp, err, "failed to init SD >>> card: %s"); >>> | >>> ~^ >>> | >>> | >>> | >>> char * >>> >>> Fixes: 4b5766488f ("Fix use of error_prepend() with &error_fatal, >>> &error_abort") >>> Inspired-by: Stefan Weil <[email protected]> >>> Signed-off-by: Philippe Mathieu-Daudé <[email protected]> >>> --- >>> include/qapi/error.h | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/include/qapi/error.h b/include/qapi/error.h >>> index 7932594dce..eeeef1a34d 100644 >>> --- a/include/qapi/error.h >>> +++ b/include/qapi/error.h >>> @@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error >>> *local_err); >>> * error_propagate(dst_errp, local_err); >>> * Please use ERRP_GUARD() and error_prepend() instead when possible. >>> */ >>> +GCC_FMT_ATTR(3, 4) >>> void error_propagate_prepend(Error **dst_errp, Error *local_err, >>> const char *fmt, ...);
Wait! You put the attribute in an unusual place. We have it at the end of the declaration elsewhere in this file. Let's remain locally consistent. >> Reviewed-by: Stefan Weil <[email protected]> >> >> error_vprepend is one more candidate for GCC_FMT_ATTR. Maybe you can add >> that, too. > > This one is different as it uses a va_list. Now I realize it is > only called in util/error.c, and all its callers are guarded with > GCC_FMT_ATTR. Maybe we can make it static to simplify... Markus? Could use GCC_FMT_ATTR(2, 0). Much less useful, but why not. Perhaps a respin would be cleaner than me applying multiple tweaks.
