Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
* Dr. David Alan Gilbert (git) (dgilb...@redhat.com) wrote:
> From: "Dr. David Alan Gilbert"
>
> If the migration fails before the channel is open (e.g. a bad
> address) we end up in the cleanup with rdma->channel==NULL.
>
> Spotted by Coverity: CID 1398634
> Fixes: fbbaacab2758cb3f32a0
> Signed-off-by: Dr. David Alan Gilbert
Queued
> ---
> migration/rdma.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 54a3c11540..9fa3b176eb 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
> rdma->connected = false;
> }
>
> -qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +if (rdma->channel) {
> +qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +}
> g_free(rdma->dest_blocks);
> rdma->dest_blocks = NULL;
>
> --
> 2.20.1
>
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
On Fri, Feb 15, 2019 at 11:00:56AM +, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Thu, Feb 14, 2019 at 06:53:51PM +, Dr. David Alan Gilbert (git)
> > wrote:
> > > From: "Dr. David Alan Gilbert"
> > >
> > > If the migration fails before the channel is open (e.g. a bad
> > > address) we end up in the cleanup with rdma->channel==NULL.
> > >
> > > Spotted by Coverity: CID 1398634
> > > Fixes: fbbaacab2758cb3f32a0
> > > Signed-off-by: Dr. David Alan Gilbert
> > > ---
> > > migration/rdma.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > index 54a3c11540..9fa3b176eb 100644
> > > --- a/migration/rdma.c
> > > +++ b/migration/rdma.c
> > > @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
> > > rdma->connected = false;
> > > }
> > >
> > > -qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> > > +if (rdma->channel) {
> > > +qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> > > +}
> >
> > IIUC there's no strict ordering constraint on resetting the fd
> > handler, then how about simply moving this line into the below "if
> > (rdma->channel)" altogether?
>
> The logic around the closing of the return path makes that check later a
> bit messy; rdma->channel can get set to Null before the other check.
Ah I see, it's the mess by sharing listen_id and channel on
destination side... Maybe we can clean them up along the way? I gave
it a shot:
if (rdma->listen_id && rdma->is_return_path) {
/*
* The return path on the destination side, both listen_id and
* channel are shared with the other context so we skip
* freeing those but simply clear the pointers no matter what.
* The main context will help us to clean these.
*/
rdma->listen_id = NULL;
rdma->channel = NULL;
} else {
/*
* Either the source side, or the main context of the
* destination side: we are responsible for listen_id/channel
*/
if (rdma->listen_id) {
rdma_destroy_id(rdma->listen_id);
rdma->listen_id = NULL;
}
if (rdma->channel) {
qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
rdma_destroy_event_channel(rdma->channel);
rdma->channel = NULL;
}
}
I slightly prefer to clean it up (if someone is still going to
maintain the RDMA code... :), but either way is fine to me.
No matter what you prefer:
Reviewed-by: Peter Xu
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
On 2/14/19 7:53 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert"
>
> If the migration fails before the channel is open (e.g. a bad
> address) we end up in the cleanup with rdma->channel==NULL.
>
> Spotted by Coverity: CID 1398634
> Fixes: fbbaacab2758cb3f32a0
> Signed-off-by: Dr. David Alan Gilbert
> ---
> migration/rdma.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 54a3c11540..9fa3b176eb 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
> rdma->connected = false;
> }
>
> -qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +if (rdma->channel) {
> +qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +}
> g_free(rdma->dest_blocks);
> rdma->dest_blocks = NULL;
>
>
Reviewed-by: Philippe Mathieu-Daudé
Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
* Peter Xu (pet...@redhat.com) wrote:
> On Thu, Feb 14, 2019 at 06:53:51PM +, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert"
> >
> > If the migration fails before the channel is open (e.g. a bad
> > address) we end up in the cleanup with rdma->channel==NULL.
> >
> > Spotted by Coverity: CID 1398634
> > Fixes: fbbaacab2758cb3f32a0
> > Signed-off-by: Dr. David Alan Gilbert
> > ---
> > migration/rdma.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 54a3c11540..9fa3b176eb 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
> > rdma->connected = false;
> > }
> >
> > -qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> > +if (rdma->channel) {
> > +qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> > +}
>
> IIUC there's no strict ordering constraint on resetting the fd
> handler, then how about simply moving this line into the below "if
> (rdma->channel)" altogether?
The logic around the closing of the return path makes that check later a
bit messy; rdma->channel can get set to Null before the other check.
Dave
> Regards,
>
> --
> Peter Xu
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
On Thu, Feb 14, 2019 at 06:53:51PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert"
>
> If the migration fails before the channel is open (e.g. a bad
> address) we end up in the cleanup with rdma->channel==NULL.
>
> Spotted by Coverity: CID 1398634
> Fixes: fbbaacab2758cb3f32a0
> Signed-off-by: Dr. David Alan Gilbert
> ---
> migration/rdma.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 54a3c11540..9fa3b176eb 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
> rdma->connected = false;
> }
>
> -qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +if (rdma->channel) {
> +qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +}
IIUC there's no strict ordering constraint on resetting the fd
handler, then how about simply moving this line into the below "if
(rdma->channel)" altogether?
Regards,
--
Peter Xu
