include/qapi/error.h | 1 + 1 file changed, 1 insertion(+)
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 <sw@weilnetz.de>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
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, ...);
--
2.21.3
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 <sw@weilnetz.de> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > 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, ...); > Reviewed-by: Stefan Weil <sw@weilnetz.de> error_vprepend is one more candidate for GCC_FMT_ATTR. Maybe you can add that, too. Regards, Stefan
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 <sw@weilnetz.de> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> 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, ...); >> > > > Reviewed-by: Stefan Weil <sw@weilnetz.de> > > 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? > > Regards, > > Stefan > >
Philippe Mathieu-Daudé <philmd@redhat.com> 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 <sw@weilnetz.de> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> 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 <sw@weilnetz.de> >> >> 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.
On 7/23/20 5:04 AM, Philippe Mathieu-Daudé wrote: >> >> 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? Using GCC_FMT_ATTR on va_list functions is just fine; the difference is that you spell its parameters (n, 0) instead of (n, n + 1). As for marking the function static, that was just discussed and rejected: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg06730.html -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > 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 s/catched/caught/ > 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 * Suggest to shorten to This would have caught the bug fixed in the previous commit. and make sure Stefan's fix actually becomes the previous commit. > Fixes: 4b5766488f ("Fix use of error_prepend() with &error_fatal, &error_abort") Kind of. Omitting the attritbute isn't wrong, just foolish / careless. I'd write Missed in commit 4b5766488f "error: Fix use of error_prepend() with &error_fatal, &error_abort". > Inspired-by: Stefan Weil <sw@weilnetz.de> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> If you like my tweaks, I can apply them in my tree. > --- > 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, ...); Reviewed-by: Markus Armbruster <armbru@redhat.com>
© 2016 - 2024 Red Hat, Inc.