| Message ID | 20200704163927.28188-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,1/5] crypto: Add tls-cipher-suites object | expand |
On Sat, 4 Jul 2020 at 17:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > The 'gen_id' argument refers to a QOM object able to produce > data consumable by the fw_cfg device. The producer object must > implement the FW_CFG_DATA_GENERATOR interface. > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > Message-Id: <20200623172726.21040-4-philmd@redhat.com> Coverity points out (CID 1430396) an issue with the error handling in this patch: > @@ -2052,6 +2056,15 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > if (nonempty_str(str)) { > size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ > buf = g_memdup(str, size); > + } else if (nonempty_str(gen_id)) { > + Error *local_err = NULL; We set local_err to NULL here... > + > + fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); ...but we don't pass it to the function here... > + if (local_err) { ...so this condition is always false and the body of the if is dead code. > + error_propagate(errp, local_err); > + return -1; > + } > + return 0; > } else { > GError *err = NULL; > if (!g_file_get_contents(file, &buf, &size, &err)) { thanks -- PMM
On 07/13/20 15:13, Peter Maydell wrote: > On Sat, 4 Jul 2020 at 17:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> The 'gen_id' argument refers to a QOM object able to produce >> data consumable by the fw_cfg device. The producer object must >> implement the FW_CFG_DATA_GENERATOR interface. >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >> Message-Id: <20200623172726.21040-4-philmd@redhat.com> > > Coverity points out (CID 1430396) an issue with the error handling > in this patch: > > >> @@ -2052,6 +2056,15 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) >> if (nonempty_str(str)) { >> size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ >> buf = g_memdup(str, size); >> + } else if (nonempty_str(gen_id)) { >> + Error *local_err = NULL; > > We set local_err to NULL here... > >> + >> + fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); > > ...but we don't pass it to the function here... Ugh, I should have noticed that in review. I'm sorry. Laszlo > >> + if (local_err) { > > ...so this condition is always false and the body of the if is dead code. > >> + error_propagate(errp, local_err); >> + return -1; >> + } >> + return 0; >> } else { >> GError *err = NULL; >> if (!g_file_get_contents(file, &buf, &size, &err)) { > > thanks > -- PMM >
On Mon, 13 Jul 2020 at 15:50, Laszlo Ersek <lersek@redhat.com> wrote:
> Ugh, I should have noticed that in review. I'm sorry.
No worries, catching this kind of thing is what static
analysers are for :-)
-- PMM
On 7/13/20 4:50 PM, Laszlo Ersek wrote: > On 07/13/20 15:13, Peter Maydell wrote: >> On Sat, 4 Jul 2020 at 17:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>> >>> The 'gen_id' argument refers to a QOM object able to produce >>> data consumable by the fw_cfg device. The producer object must >>> implement the FW_CFG_DATA_GENERATOR interface. >>> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >>> Message-Id: <20200623172726.21040-4-philmd@redhat.com> >> >> Coverity points out (CID 1430396) an issue with the error handling >> in this patch: >> >> >>> @@ -2052,6 +2056,15 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) >>> if (nonempty_str(str)) { >>> size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ >>> buf = g_memdup(str, size); >>> + } else if (nonempty_str(gen_id)) { >>> + Error *local_err = NULL; >> >> We set local_err to NULL here... >> >>> + >>> + fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); >> >> ...but we don't pass it to the function here... > > Ugh, I should have noticed that in review. I'm sorry. You reviewed v9 which was OK, I added it while addressing Daniel comment for v10, while keeping your R-b tag from v9. Since you already had reviewed 9 different versions, I didn't want to overload you for a trivial change, but I should have dropped your tag to be certain. Also this has been merged at the same time Markus was doing a big rework on the Error API, so I was very confused between reviews of the new API. So don't be sorry, Daniel/Myself let that in ;) I'll send a patch, hoping it can be queued via qemu-trivial. Regards, Phil. > Laszlo > >> >>> + if (local_err) { >> >> ...so this condition is always false and the body of the if is dead code. >> >>> + error_propagate(errp, local_err); >>> + return -1; >>> + } >>> + return 0; >>> } else { >>> GError *err = NULL; >>> if (!g_file_get_contents(file, &buf, &size, &err)) { >> >> thanks >> -- PMM >> >
diff --git a/softmmu/vl.c b/softmmu/vl.c index 3e15ee2435..13cada39d6 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -489,6 +489,11 @@ static QemuOptsList qemu_fw_cfg_opts = { .name = "string", .type = QEMU_OPT_STRING, .help = "Sets content of the blob to be inserted from a string", + }, { + .name = "gen_id", + .type = QEMU_OPT_STRING, + .help = "Sets id of the object generating the fw_cfg blob " + "to be inserted", }, { /* end of list */ } }, @@ -2020,7 +2025,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) { gchar *buf; size_t size; - const char *name, *file, *str; + const char *name, *file, *str, *gen_id; FWCfgState *fw_cfg = (FWCfgState *) opaque; if (fw_cfg == NULL) { @@ -2030,14 +2035,13 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) name = qemu_opt_get(opts, "name"); file = qemu_opt_get(opts, "file"); str = qemu_opt_get(opts, "string"); + gen_id = qemu_opt_get(opts, "gen_id"); - /* we need name and either a file or the content string */ - if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) { - error_setg(errp, "invalid argument(s)"); - return -1; - } - if (nonempty_str(file) && nonempty_str(str)) { - error_setg(errp, "file and string are mutually exclusive"); + /* we need the name, and exactly one of: file, content string, gen_id */ + if (!nonempty_str(name) || + nonempty_str(file) + nonempty_str(str) + nonempty_str(gen_id) != 1) { + error_setg(errp, "name, plus exactly one of file," + " string and gen_id, are needed"); return -1; } if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) { @@ -2052,6 +2056,15 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) if (nonempty_str(str)) { size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ buf = g_memdup(str, size); + } else if (nonempty_str(gen_id)) { + Error *local_err = NULL; + + fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); + if (local_err) { + error_propagate(errp, local_err); + return -1; + } + return 0; } else { GError *err = NULL; if (!g_file_get_contents(file, &buf, &size, &err)) {