Philippe Mathieu-Daudé <f4...@amsat.org> writes: > +Peter for crediting his advice. > > On 4/29/20 7:59 AM, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <f4...@amsat.org> writes: >> >>> On 4/24/20 9:20 PM, Markus Armbruster wrote: >>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a >>>> pointer to a variable containing NULL. Passing an argument of the >>>> latter kind twice without clearing it in between is wrong: if the >>>> first call sets an error, it no longer points to NULL for the second >>>> >>>> create_cps() is wrong that way. The last calls treats an error as >>>> fatal. Do that for the prior ones, too. >>>> >>>> Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c >>>> Cc: Aleksandar Markovic <aleksandar.qemu.de...@gmail.com> >>>> Cc: "Philippe Mathieu-Daudé" <phi...@redhat.com> >>>> Cc: Aurelien Jarno <aurel...@aurel32.net> >>>> Signed-off-by: Markus Armbruster <arm...@redhat.com> >>>> --- >>>> hw/mips/mips_malta.c | 15 ++++++--------- >>>> 1 file changed, 6 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c >>>> index e4c4de1b4e..17bf41616b 100644 >>>> --- a/hw/mips/mips_malta.c >>>> +++ b/hw/mips/mips_malta.c >>>> @@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState >>>> *ms, >>>> static void create_cps(MachineState *ms, MaltaState *s, >>>> qemu_irq *cbus_irq, qemu_irq *i8259_irq) >>>> { >>>> - Error *err = NULL; >>>> - >>>> sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), >>>> sizeof(s->cps), >>>> TYPE_MIPS_CPS); >>>> - object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", >>>> &err); >>>> - object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", >>>> &err); >>>> - object_property_set_bool(OBJECT(&s->cps), true, "realized", &err); >>>> - if (err != NULL) { >>>> - error_report("%s", error_get_pretty(err)); >>> >>> In https://www.mail-archive.com/qemu-devel@nongnu.org/msg695645.html I >>> also remove "qemu/error-report.h" which is not needed once you remove >>> this call. >> >> Missed it, sorry. I only reviewed the Coccinelle scripts [PATCH 1+3/7]. > > My bad for not Cc'ing you on the whole series, which is Error related, > and use the default get_maintainer.pl selection. > >> I'd replace my patch by yours to give you proper credit, but your commit >> message mentions "the coccinelle script", presumably the one from PATCH >> 1/7, and that patch isn't quite ready in my opinion. > > I'm not worried about credit, but duplicating effort or wasting time. > > Peter once warned the problem with Coccinelle scripts is finding the > correct balance between time spent to improve QEMU codebase, and time > spent learning Coccinelle and improving a script that is often used > only once in a lifetime. > If the script is not provided, we ask for the script. If the script is > embedded in various patch descriptions, we ask to add it independently > for reuse or as example. Then the script must be almost > perfect. Meanwhile all the following patches referencing it, while > reviewed, are stuck.
True. I try not to ask for undue perfection, but perfection eludes me in that, too :) For PATCH 1/7, I only asked you to explain the script's limitations in the script and the commit message. Her's a bit of inspiration from the kernel's scripts/coccinelle/misc/doubleinit.cocci (picked almost at random): /// Find duplicate field initializations. This has a high rate of false /// positives due to #ifdefs, which Coccinelle is not aware of in a structure /// initialization. /// // Confidence: Low I like the Confidence: tag. It should come with an explanation, as it does here. For PATCH 3/7, I had a question. > Anyway back to your patch: > Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org> > Tested-by: Philippe Mathieu-Daudé <f4...@amsat.org> Thanks!