On Thu, 3 Apr 2025 at 18:29, Fabiano Rosas <faro...@suse.de> wrote:
> Yes, there's no point. if we already have main and multifd channels,
> what's left must be postcopy.
* Okay.
> Well, but don't add it blindly if it doesn't make sense.
* Hmmn, okay. When I say/do things that seem reasonable to me, I'm
told - it's much easier to just do things than arguing over them. When
I do something because it's a minor change, I'm admonished with this.
Life is tricky. :)
> 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;
> }
* Okay.
> > * 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.
* Okay.
* If we add assert(!mis->from_src_file), then the if
(!mis->from_src_file) check in migration_incoming_setup() is not
needed then.
> 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.
* Okay. I'll add an assert(migrate_multifd()). That should be
consistent with the other assert(!mis->from_src_file),
assert(migrate_postcopy_preempt()) and
assert(!mis->postcopy_qemufile_dst) calls in there.
Thank you.
---
- Prasad
