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

[Li Zhang]

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.