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

[Peter Maydell]

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.)

thanks
-- PMM