Re: [Qemu-devel] [PATCH v6 5/8] tmp backend: Add new api to read backend TpmInfo

[Eric Blake]

On 07/18/2017 05:39 AM, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jul 18, 2017 at 1:49 AM, Amarnath Valluri
> <address@hidden> wrote:
>> TPM configuration options are backend implementation details and shall not be
>> part of base TPMBackend object, and these shall not be accessed directly 
>> outside
>> of the class, hence added a new interface method, get_tpm_options() to
>> TPMDriverOps., which shall be implemented by the derived classes to return
>> configured tpm options.
>>
> One usually prefer to have the true case first.
> 
>> +    } else {
>> +        tpm_pt->ops->has_path = true;
>>      }
>>
>> +    tpm_pt->ops->path = g_strdup(value);
> 
> Interestingly, ops->path will be set even if ops->has_path = false. I
> am not sure the visitors will handle that case properly (for visit or
> dealloc etc). Could you set ops->has_path = true uncondtionnally?

tmp_pt->opt->path is ignored if has_path is false; if it is assigned to
malloc'd memory, then you leak that memory when freeing tpm_pt.

> 
> 
> 
>>      tpm_pt->tpm_dev = g_strdup(value);
>>
>> -    tb->path = g_strdup(tpm_pt->tpm_dev);
>> -
>>      tpm_pt->tpm_fd = qemu_open(tpm_pt->tpm_dev, O_RDWR);
>>      if (tpm_pt->tpm_fd < 0) {
>>          error_report("Cannot access TPM device using '%s': %s",
>> @@ -382,8 +389,8 @@ static int tpm_passthrough_handle_device_opts(QemuOpts 
>> *opts, TPMBackend *tb)
>>      tpm_pt->tpm_fd = -1;
>>
>>   err_free_parameters:
>> -    g_free(tb->path);
>> -    tb->path = NULL;
>> +    g_free(tpm_pt->ops->path);
>> +    tpm_pt->ops->path = NULL;
>>
> 
> Shouldn't it also free cancel_path?

Even better, don't open-code it.  Use
qapi_free_TPMPassthruState(tpm_pt), so that the generated code gets
everything for you.


>> +static TpmTypeOptions *tpm_passthrough_get_tpm_options(TPMBackend *tb)
>> +{
>> +    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
>> +    TpmTypeOptions *ops = NULL;
>> +    TPMPassthroughOptions *pops = NULL;
>> +
>> +    pops = g_new0(TPMPassthroughOptions, 1);
>> +    if (!pops) {
>> +        return NULL;
>> +    }
>> +
> 
> There is no check for small allocation failure elsewhere, I would drop that.

In fact, g_new0() can't fail (if you're out of memory, it abort()s
instead of returning NULL).  Coverity will warn about dead code.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature