Re: [Qemu-devel] [PATCH v2 2/3] migration: fix memory leak when updating tls-creds and tls-hostname

[Peter Xu]

On Tue, Jan 15, 2019 at 10:03:53AM -0600, Eric Blake wrote:
> On 1/15/19 4:24 AM, Dr. David Alan Gilbert wrote:
> 
> > I think the problem is that
> > migrate_params_check checks a MigrationParameters
> > 
> > while the QMP command gives us a MigrateSetParameters; but we also use
> > migrate_params_check for the global check you added (8b0b29dc) which is
> > against migrationParameters; so that's why migrate_params_check takes
> > a MigrationParameters.
> > 
> > It's horrible we've got stuff duped so much.
> 
> Indeed.
> 
> > 
> > However, I don't like this fix because if someone later was to add
> > a test for tls parameters to migrate_params_check, then they would be
> > confused why the hostname/creds weren't checked.
> > So while we have migrate_params_test_apply, it should cover all
> > parameters.
> > 
> > I think a cleaner check would be to write a MigrateParameters_free
> > that free'd any strings, and call that in qmp_migrate_set_parameters
> > on both exit paths.
> 
> We already have it; it's named qapi_free_MigrationParameters(),
> generated in qapi-types-migration.h.

Yes this seems better.  Then IIUC patch 3 can be simplified as well
with it.

I'm doing one step back and reading below thread for more context that
I've missed:

  https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04526.html

Do we have chance/plan to remove these duplication for QEMU 4.0?

Thanks,

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




Regards,

-- 
Peter Xu