On Wed, 29 Apr 2020 at 08:34, Markus Armbruster <arm...@redhat.com> 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 > call. > > configure_icount() is wrong that way. Harmless, because its @errp is > always &error_abort or &error_fatal. > > Just as wrong (and just as harmless): when it fails, it can still > update global state.
Hi; Coverity complains about this change (CID 1428754): > > void configure_icount(QemuOpts *opts, Error **errp) > { > - const char *option; > + const char *option = qemu_opt_get(opts, "shift"); > + bool sleep = qemu_opt_get_bool(opts, "sleep", true); > + bool align = qemu_opt_get_bool(opts, "align", false); > + long time_shift = -1; > char *rem_str = NULL; > > - option = qemu_opt_get(opts, "shift"); > - if (!option) { > - if (qemu_opt_get(opts, "align") != NULL) { > - error_setg(errp, "Please specify shift option when using align"); > - } > + if (!option && qemu_opt_get(opts, "align")) { > + error_setg(errp, "Please specify shift option when using align"); > return; Previously, if option was NULL we would always take this early exit. Now we only take the exit if option is NULL and the qemu_opt_get() returns true, so in some cases execution can continue through the function with a NULL option... > } > > - icount_sleep = qemu_opt_get_bool(opts, "sleep", true); > + if (align && !sleep) { > + error_setg(errp, "align=on and sleep=off are incompatible"); > + return; > + } > + > + if (strcmp(option, "auto") != 0) { ...but here we pass option to strcmp(), which is wrong if it can be NULL. thanks -- PMM