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

[Li Zhang]

On 1/27/22 10:53 AM, Juan Quintela wrote:
Li Zhang <lizh...@suse.de> wrote:
Clean up some unnecessary code
Signed-off-by: Li Zhang <lizh...@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.


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.