Re: [PULL 3/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument

From: Laszlo Ersek <lersek@redhat.com>
To: "Peter Maydell" <peter.maydell@linaro.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>
Subject: Re: [PULL 3/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
Date: Mon, 13 Jul 2020 16:50:00 +0200	[thread overview]
Message-ID: <17fd7aea-cc7e-c17a-5748-88dd0d106317@redhat.com> (raw)
In-Reply-To: <CAFEAcA8v2rEC0T_=D1Bat9DQJ=YS57fRUDDQyFdxJsEB=OEPMw@mail.gmail.com>

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
> 