| Message ID | 20211217101228.9512-1-lizhang@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] multifd: Remove some redundant code | expand |
ping Any comments? Thanks Li On 12/17/21 11:12 AM, Li Zhang wrote: > Clean up some unnecessary code > > Signed-off-by: Li Zhang <lizhang@suse.de> > --- > 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); > } > > @@ -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; > } > >
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? 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.
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. >
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. 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. >
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. >> >
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. >>> >> > >
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); } @@ -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; }
Clean up some unnecessary code Signed-off-by: Li Zhang <lizhang@suse.de> --- migration/multifd.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)