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

[Philippe Mathieu-Daudé]

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
>>
>