On 07/18/2017 05:39 AM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jul 18, 2017 at 1:49 AM, Amarnath Valluri
> <amarnath.vall...@intel.com> 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
