On Mon, Dec 16, 2024 at 12:58:58PM -0300, Fabiano Rosas wrote:
> > @@ -3286,6 +3237,11 @@ static void
> > migration_iteration_finish(MigrationState *s)
> > case MIGRATION_STATUS_FAILED:
> > case MIGRATION_STATUS_CANCELLED:
> > case MIGRATION_STATUS_CANCELLING:
>
> Pre-existing, but can we even reach here with CANCELLED? If we can start
> the VM with both CANCELLED and CANCELLING, that means the
> MIG_EVENT_PRECOPY_FAILED notifier is not being consistently called. So I
> think CANCELLED here must be unreachable...
Yeah I can't see how it's reachable, because the only place that we can set
it to CANCELLED is:
migrate_fd_cleanup():
if (s->state == MIGRATION_STATUS_CANCELLING) {
migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
MIGRATION_STATUS_CANCELLED);
}
In this specific context, it (as a bottom half) can only be scheduled after
this path.
Looks like the event MIG_EVENT_PRECOPY_FAILED will work regardless, though?
As that's also done after above:
type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED :
MIG_EVENT_PRECOPY_DONE;
migration_call_notifiers(s, type, NULL);
So looks like no matter it was CANCELLING or CANCELLED, it'll always be
CANCELLED when reaching migration_has_failed().
[...]
> > @@ -103,13 +104,8 @@ void qmp_cont(Error **errp)
> > * Continuing after completed migration. Images have been
> > * inactivated to allow the destination to take control. Need to
> > * get control back now.
> > - *
> > - * If there are no inactive block nodes (e.g. because the VM was
> > - * just paused rather than completing a migration),
> > - * bdrv_inactivate_all() simply doesn't do anything.
> > */
> > - bdrv_activate_all(&local_err);
> > - if (local_err) {
> > + if (!migration_block_activate(&local_err)) {
> > error_propagate(errp, local_err);
>
> Could use errp directly here.
True..
--
Peter Xu
