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

[Maxim Davydov <maxim.davydov@openvz.org>]

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