[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] postocpy: Check that postocpy fd's are not NULL
From: |
Juan Quintela |
Subject: |
Re: [PATCH] postocpy: Check that postocpy fd's are not NULL |
Date: |
Wed, 03 Nov 2021 21:10:07 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
"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
>>