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

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "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

True we could indeed fix both as a belt-and-braces.

> > (Also note 'post*o*cpy' typo in title, and probably worth a
> >   Fixes: c4f7538 ?)
> 
> Sure.
> 
> What do you think?

So yeh,

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Please do the other if as a separate patch sometime.

Dave

> 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
> >> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK