Re: [PATCH v1 7/9] colo-compare: safe finalization

[Maxim Davydov]

The main problem that if we call object_new_with_class() and then 
object_unref(), it fails. First of all, this is due to the fact that 
finalize expects that net/colo-compare.c:colo_compare_complete() has 
been called before.

On 3/30/22 17:54, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2022 00:15, Maxim Davydov wrote:
> > Fixes some possible issues with finalization. For example, finalization
> > immediately after instance_init fails on the assert.
> >
> > Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> > ---
> >   net/colo-compare.c | 25 ++++++++++++++++---------
> >   1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c
> > index 62554b5b3c..81d8de0aaa 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -1426,7 +1426,7 @@ static void colo_compare_finalize(Object *obj)
> >               break;
> >           }
> >       }
> > -    if (QTAILQ_EMPTY(&net_compares)) {
if colo_compare_active == false, event_mtx and event_complete_cond 
didn't inited in colo_compare_complete()
> > +    if (QTAILQ_EMPTY(&net_compares) && colo_compare_active) {
> >           colo_compare_active = false;
> >           qemu_mutex_destroy(&event_mtx);
> >           qemu_cond_destroy(&event_complete_cond);
> > @@ -1442,19 +1442,26 @@ static void colo_compare_finalize(Object *obj)
> >         colo_compare_timer_del(s);
> >   -    qemu_bh_delete(s->event_bh);
s->event_bh wasn't allocated in colo_compare_iothread() in 
colo_compare_complete()
> > +    if (s->event_bh) {
> > +        qemu_bh_delete(s->event_bh);
> > +    }
> >   -    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > -    aio_context_acquire(ctx);
> > -    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > -    if (s->notify_dev) {
> > -        AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
s->iothread == NULL after .instance_init (it can be detected in 
colo_compare_complete(), if it has been called)
> > +    if (s->iothread) {
> > +        AioContext *ctx = iothread_get_aio_context(s->iothread);
> > +        aio_context_acquire(ctx);
> > +        AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > +        if (s->notify_dev) {
> > +            AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
> > +        }
> > +        aio_context_release(ctx);
> >       }
> > -    aio_context_release(ctx);
> >         /* Release all unhandled packets after compare thead exited */
> >       g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> > -    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
In normal situation, it flushes all packets and sets s->out_sendco.done 
= true via compare_chr_send (we wait this event). But s->conn_list isn't 
initialized, s->out_sendco.done == false and won't become true. So, it's 
infinite waiting.
> > +    /* Without colo_compare_complete done == false without packets */
> > +    if (!g_queue_is_empty(&s->out_sendco.send_list)) {
> > +        AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
> > +    }
>
> I think, would be good to add more description for this last change. 
> It's not as obvious as previous two changes.
>
> > g_queue_clear(&s->conn_list);
> >       g_queue_clear(&s->out_sendco.send_list);
--
Best regards,
Maxim Davydov