Prasad Pandit <ppan...@redhat.com> writes:
> Hello Fabiano,
>
> On Mon, 31 Mar 2025 at 20:31, Fabiano Rosas <faro...@suse.de> wrote:
>> > + } else if (mis->from_src_file) {
>> This is redundant.
>
> * This was to ensure (double check) that when the Postcopy connection
> comes in, the main channel is established. Also a couple of versions
> back migration qtest was failing without this check. Nonetheless,
> qtests do work now without this check. I'll remove it if we must.
>
Yes, there's no point. if we already have main and multifd channels,
what's left must be postcopy.
>> > + channel = CH_POSTCOPY;
>> > } else {
>> > - default_channel = !mis->from_src_file;
>> > + channel = CH_MAIN;
>>
>> And this is impossible.
>
> ->
> https://lore.kernel.org/qemu-devel/20250215123119.814345-1-ppan...@redhat.com/T/#m18b6bf30e877f9eafaa67bba6a209b47782f6eac
>
> * Yes, but a couple of revisions back you suggested adding it saying
> CH_MAIN assignment at the top was doing some heavy lifting and it's
> more clear this way.
>
Well, but don't add it blindly if it doesn't make sense. The point was
to not end the conditional at 'else if' because that makes the reader
have to go look around the code to see what was already assigned. Here
we want just a plain:
else {
channel = CH_POSTCOPY;
}
>> We should probably expand migration_incoming_setup() to make it clear
>> that mis->from_src_file is set at this point. And
>> assert(!mis->from_src_file). I can send a patch on top later.
>
> * migration_incoming_setup uses the QEMUFile object only when
> mis->from_src_file is not set. I'm wondering if we really need an
> assert(!mis->from_src_file) check? Because it'll reach here only when
> channel == CH_MAIN and channel is set to CH_MAIN only when
> mis->from_src_file is NULL.
>
>
Given the:
if (!mis->from_src_file) {
I think someone (back in 2017) thought it was possible to reach there
with from_src_file already set. I don't know whether that applied to
this path. In any case, for this function I believe the correct is
assert because we shouldn't have two channels arriving as main.
>> > - } else {
>> > + } else if (channel == CH_MULTIFD) {
>> > /* Multiple connections */
>> > - assert(migration_needs_multiple_sockets());
>> > if (migrate_multifd()) {
>>
>> This should be an assert.
>
> Same, 'channel' is set to CH_MULTIFD, only when migrate_multifd() is
> enabled. Do we need another assert(migrate_multifd()) check?
>
Maybe not, but we definitely cannot just ignore if it happens and we
also should not have an empty check that is always known to be true. So
either assert or remove the if entirely.
>> > + } else if (channel == CH_POSTCOPY) {
>> > + assert(migrate_postcopy_preempt());
>> > + assert(!mis->postcopy_qemufile_dst);
>> > + f = qemu_file_new_input(ioc);
>> > + postcopy_preempt_new_channel(mis, f);
>> > + return;
>> > }
>> >
>> > - if (migration_should_start_incoming(default_channel)) {
>> > + if (migration_has_main_and_multifd_channels()) {
>>
>> I think there's a bug here. Excluding multifd from the picture, if only
>> the main channel needs to be setup, then it's possible to start postcopy
>> recovery twice, once when the main channel appears and another time when
>> the preempt channel appears.
>
> * When the preempt channel appears 'channel' is set to CH_POSTCOPY, so
> it shall 'return' before reaching here, right?
>
You're right, I missed the return statement.
> ===
> } else if (!mis->from_src_file &&
> mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> /* reconnect main channel for postcopy recovery */
> channel = CH_MAIN;
> } else {
> ===
> * When 'main' channel connection arrives for postcopy recovery,
> 'channel' shall be set to CH_MAIN.
>
>> The previous code worked differently because it did:
>>
>> if (migrate_postcopy_preempt()) {
>> return main_channel;
>>
>> which would return false when preempt arrived after main.
>
> * Yes.
>
>> We could use migration_has_all_channels() instead, that would look more
>> logically correct, but it would also change the current behavior that
>> postcopy recovery can start before the preempt channel is in place. I'm
>> not even sure if that's actually part of the design of the feature.
>
> * Not sure if we need this.
>
> Thank you.
> ---
> - Prasad
