Re: [PATCH] postocpy: Check that postocpy fd's are not NULL

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> If postcopy has finished, it frees the array.
>> But vhost-user unregister it at cleanup time.
>
> I must admit to being confused as the double migrate case vs migrate
> once and shutdown. It might just be an ordering thing?
>
> I notice that  'vhost_user_backend_cleanup' does:
>     if (u->postcopy_fd.handler) {
>         postcopy_unregister_shared_ufd(&u->postcopy_fd);
>         close(u->postcopy_fd.fd);
>         u->postcopy_fd.handler = NULL;
>     }
>
> where as the other caller, 'vhost_user_postcopy_end'
> does:
>     postcopy_unregister_shared_ufd(&u->postcopy_fd);
>     close(u->postcopy_fd.fd);
>     u->postcopy_fd.handler = NULL;

I think that we want ta make here the check to see if it has already
been freed.

> maybe it would be better to fix them to do the same if check?

But even there, I think that it is more robust that we don't try to
access a NULL pointer.

I.e. there are two things that we can fix here:
- postcopy unregister
- vhost use of postcopy unregister

> (Also note 'post*o*cpy' typo in title, and probably worth a
>   Fixes: c4f7538 ?)

Sure.

What do you think?

Later, Juan.

> Dave
>
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/postcopy-ram.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index e721f69d0f..d18b5d05b2 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -1457,6 +1457,10 @@ void postcopy_unregister_shared_ufd(struct PostCopyFD *pcfd)
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>>      GArray *pcrfds = mis->postcopy_remote_fds;
>>  
>> +    if (!pcrfds) {
>> +        /* migration has already finished and freed the array */
>> +        return;
>> +    }
>>      for (i = 0; i < pcrfds->len; i++) {
>>          struct PostCopyFD *cur = &g_array_index(pcrfds, struct PostCopyFD, i);
>>          if (cur->fd == pcfd->fd) {
>> -- 
>> 2.33.1
>>