* [PATCH v2 1/1] multifd: Remove some redundant code
@ 2021-12-17 10:12 Li Zhang
2022-01-06 10:06 ` Li Zhang
2022-01-27 9:53 ` Juan Quintela
0 siblings, 2 replies; 7+ messages in thread
From: Li Zhang @ 2021-12-17 10:12 UTC (permalink / raw)
To: quintela, dgilbert, cfontana, qemu-devel; +Cc: Li Zhang
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;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] multifd: Remove some redundant code
2021-12-17 10:12 [PATCH v2 1/1] multifd: Remove some redundant code Li Zhang
@ 2022-01-06 10:06 ` Li Zhang
2022-01-27 9:53 ` Juan Quintela
1 sibling, 0 replies; 7+ messages in thread
From: Li Zhang @ 2022-01-06 10:06 UTC (permalink / raw)
To: quintela, dgilbert, cfontana, qemu-devel
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;
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] multifd: Remove some redundant code
2021-12-17 10:12 [PATCH v2 1/1] multifd: Remove some redundant code Li Zhang
2022-01-06 10:06 ` Li Zhang
@ 2022-01-27 9:53 ` Juan Quintela
2022-01-27 9:56 ` Li Zhang
2022-01-27 18:11 ` Li Zhang
1 sibling, 2 replies; 7+ messages in thread
From: Juan Quintela @ 2022-01-27 9:53 UTC (permalink / raw)
To: Li Zhang; +Cc: qemu-devel, dgilbert, cfontana
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] multifd: Remove some redundant code
2022-01-27 9:53 ` Juan Quintela
@ 2022-01-27 9:56 ` Li Zhang
2022-01-27 18:11 ` Li Zhang
1 sibling, 0 replies; 7+ messages in thread
From: Li Zhang @ 2022-01-27 9:56 UTC (permalink / raw)
To: quintela; +Cc: qemu-devel, dgilbert, cfontana
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.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] multifd: Remove some redundant code
2022-01-27 9:53 ` Juan Quintela
2022-01-27 9:56 ` Li Zhang
@ 2022-01-27 18:11 ` Li Zhang
2022-01-27 18:31 ` Li Zhang
1 sibling, 1 reply; 7+ messages in thread
From: Li Zhang @ 2022-01-27 18:11 UTC (permalink / raw)
To: quintela; +Cc: qemu-devel, dgilbert, cfontana
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.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] multifd: Remove some redundant code
2022-01-27 18:11 ` Li Zhang
@ 2022-01-27 18:31 ` Li Zhang
2022-01-27 19:10 ` Li Zhang
0 siblings, 1 reply; 7+ messages in thread
From: Li Zhang @ 2022-01-27 18:31 UTC (permalink / raw)
To: quintela; +Cc: qemu-devel, dgilbert, cfontana
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.
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] multifd: Remove some redundant code
2022-01-27 18:31 ` Li Zhang
@ 2022-01-27 19:10 ` Li Zhang
0 siblings, 0 replies; 7+ messages in thread
From: Li Zhang @ 2022-01-27 19:10 UTC (permalink / raw)
To: quintela; +Cc: cfontana, qemu-devel, dgilbert
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.
>>>
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-27 19:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-17 10:12 [PATCH v2 1/1] multifd: Remove some redundant code Li Zhang
2022-01-06 10:06 ` Li Zhang
2022-01-27 9:53 ` Juan Quintela
2022-01-27 9:56 ` Li Zhang
2022-01-27 18:11 ` Li Zhang
2022-01-27 18:31 ` Li Zhang
2022-01-27 19:10 ` Li Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).