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

[Li Zhang <lizhang@suse.de>]

On 1/27/22 7:31 PM, Li Zhang wrote:
> 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. :)

Please ignore my mail. I made mistakes and I will update it soon.

Thanks
LI
> 
>>
>> 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.
>>>
>>
> 
>