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

[Li Zhang <lizhang@suse.de>]

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?

Ah, this is better. thanks. :)


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