Re: [PULL 08/14] migration: add new migration state wait-unplug

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Juan Quintela <quintela@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Markus Armbruster <armbru@redhat.com>,
	Jens Freimann <jfreimann@redhat.com>
Subject: Re: [PULL 08/14] migration: add new migration state wait-unplug
Date: Mon, 29 Jun 2020 13:09:39 +0100	[thread overview]
Message-ID: <20200629120939.GI2908@work-vm> (raw)
In-Reply-To: <CAFEAcA8uSbC80a+yB4_DFtCB1_-sXW5R3ugTX6H9XDeBZV-mQQ@mail.gmail.com>

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Tue, 29 Oct 2019 at 23:00, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Jens Freimann <jfreimann@redhat.com>
> >
> > This patch adds a new migration state called wait-unplug.  It is entered
> > after the SETUP state if failover devices are present. It will transition
> > into ACTIVE once all devices were succesfully unplugged from the guest.
> >
> > So if a guest doesn't respond or takes long to honor the unplug request
> > the user will see the migration state 'wait-unplug'.
> >
> > In the migration thread we query failover devices if they're are still
> > pending the guest unplug. When all are unplugged the migration
> > continues. If one device won't unplug migration will stay in wait_unplug
> > state.
> >
> > Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Message-Id: <20191029114905.6856-9-jfreimann@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> 
> Hi; Coverity has just (rather belatedly) noticed a possible
> issue in this code (CID 1429995):
> 
> > @@ -3264,6 +3270,19 @@ static void *migration_thread(void *opaque)
> >
> >      qemu_savevm_state_setup(s->to_dst_file);
> >
> > +    if (qemu_savevm_nr_failover_devices()) {
> > +        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> > +                          MIGRATION_STATUS_WAIT_UNPLUG);
> > +
> > +        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> > +               qemu_savevm_state_guest_unplug_pending()) {
> > +            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> 
> Here we call qemu_sem_timedwait() but ignore the return value,
> whereas all the other callsites for that function do something
> with the return value. Is the code correct? (This is just a
> heuristic Coverity has, and it's wrong a fair amount of the
> time, so if it's wrong here too I can just mark it as a
> false-positive in the Coverity UI.)

Hmm it's OK; that semaphore isn't really used at the moment,
so it's pretty much just a sleep. And the loop always
calls the qemu_savevm_state_guest_unplug_pending() before
it goes around again; so it doesn't care if the return
value indicates timeout or not.

Dave

> thanks
> -- PMM
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK