Peter Maydell <peter.mayd...@linaro.org> writes: > 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.
Right. I'll post a fix. Thank you!