Re: [Qemu-devel] [PATCH v7 07/22] migration: Make migrate_fd_error() the owner of the Error

[Juan Quintela <quintela@redhat.com>]

Eric Blake <eblake@redhat.com> wrote:
D> On 09/06/2017 06:51 AM, Juan Quintela wrote:
>> So far, we had to free the error after each caller, so just do it
>> here.  Once there, tls.c was leaking the error.
>
> You mention tls.c,
>
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/channel.c   |  1 -
>>  migration/migration.c | 10 ++++------
>>  migration/migration.h |  4 ++--
>>  migration/socket.c    |  1 -
>>  4 files changed, 6 insertions(+), 10 deletions(-)
>
> but don't touch it.  Am I missing something?
>

It was missing error_free();  So it leaked the Error * variable.
I will improve the message for next version.



>>  
>> -void migrate_fd_error(MigrationState *s, const Error *error)
>> +void migrate_fd_error(MigrationState *s, Error *error)
>>  {
>
> No comments at definition,

We free it inside now, so it can't be const.

>> +++ b/migration/migration.h
>> @@ -163,8 +163,8 @@ bool  migration_has_all_channels(void);
>>  
>>  uint64_t migrate_max_downtime(void);
>>  
>> -void migrate_set_error(MigrationState *s, const Error *error);
>> -void migrate_fd_error(MigrationState *s, const Error *error);
>> +void migrate_set_error(MigrationState *s, Error *error);
>> +void migrate_fd_error(MigrationState *s, Error *error);
>
> or at declaration. That would be worth adding at some point, but this
> patch isn't making it worse.

will add them, thanks.

> The code looks okay in isolation, so if it is only the commit message
> that needs fixing,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks.