Re: [PATCH v2 1/1] multifd: Remove some redundant code

[Li Zhang <lizhang@suse.de>]

On 1/27/22 7:11 PM, Li Zhang wrote:
> On 1/27/22 10:53 AM, Juan Quintela wrote:
>> Li Zhang <lizhang@suse.de> wrote:
>>> Clean up some unnecessary code
>>>
>>> Signed-off-by: Li Zhang <lizhang@suse.de>
>>
>> Hi
>>
>>> ---
>>>   migration/multifd.c | 12 +++---------
>>>   1 file changed, 3 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/migration/multifd.c b/migration/multifd.c
>>> index 3242f688e5..212be1ed04 100644
>>> --- a/migration/multifd.c
>>> +++ b/migration/multifd.c
>>> @@ -854,19 +854,16 @@ static void 
>>> multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
>>>       Error *local_err = NULL;
>>>       trace_multifd_new_send_channel_async(p->id);
>>> -    if (qio_task_propagate_error(task, &local_err)) {
>>> -        goto cleanup;
>>> -    } else {
>>> +    if (!qio_task_propagate_error(task, &local_err)) {
>>>           p->c = QIO_CHANNEL(sioc);
>>>           qio_channel_set_delay(p->c, false);
>>>           p->running = true;
>>>           if (!multifd_channel_connect(p, sioc, local_err)) {
>>> -            goto cleanup;
>>> +            multifd_new_send_channel_cleanup(p, sioc, local_err);
>>>           }
>>>           return;
>>>       }
>>> -cleanup:
>>>       multifd_new_send_channel_cleanup(p, sioc, local_err);
>>>   }
>>
>> Once there, why are we duplicating the call to
>> multifd_new_send_channel_cleanup()
>>
>> What about:
>>
>> static void multifd_new_send_channel_async(QIOTask *task, gpointer 
>> opaque)
>> {
>>      MultiFDSendParams *p = opaque;
>>      QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
>>      Error *local_err = NULL;
>>
>>      trace_multifd_new_send_channel_async(p->id);
>>      if (!qio_task_propagate_error(task, &local_err)) {
>>          p->c = QIO_CHANNEL(sioc);
>>          qio_channel_set_delay(p->c, false);
>>          p->running = true;
>>          if (multifd_channel_connect(p, sioc, local_err)) {
>>              return;
>>          }
>>      }
>>      multifd_new_send_channel_cleanup(p, sioc, local_err);
>> }
>>
>> What do you think?
> 
> Hi Juan,
> 
> Sorry about the code. I check it again, it still needs to cleaup
> if it fails on multifd_channel_connect(). I just remove the goto
> and put the cleanup function there.
> 
> The second cleanup is called if (qio_task_propagate_error(task, 
> &local_err)) ), otherwise, it won't cleanup and return normally.
> 
> If removing the first cleanup and move the return, the logic is not 
> right.  It is that: when no error happens, it still calls the second
> cleanup.
> 

I checked source code from the qemu.
This condition is not right,
if (!multifd_channel_connect(p, sioc, local_err)) is not right.
The function multifd_channel_connect return true if no error.

The mail is right. :)

> 
> Thanks
> Li
>>
>> Later, Juan.
>>
>>
>>> @@ -1078,10 +1075,7 @@ static void *multifd_recv_thread(void *opaque)
>>>           ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
>>>                                          p->packet_len, &local_err);
>>> -        if (ret == 0) {   /* EOF */
>>> -            break;
>>> -        }
>>> -        if (ret == -1) {   /* Error */
>>> +        if (ret == 0 || ret == -1) {   /* 0: EOF  -1: Error */
>>>               break;
>>>           }
>>
>> This bit is ok.
>>
>