Re: [Qemu-block] [Qemu-devel] [PATCH v2] migration: disallow migrate_add_blocker during migration

[Eric Blake]

On 09/30/2015 11:07 AM, John Snow wrote:
> If a migration is already in progress and somebody attempts
> to add a migration blocker, this should rightly fail.
> 
> Add an errp parameter and a retcode return value to migrate_add_blocker.
> 
> This is part one of two for a solution to prohibit e.g. block jobs
> from running concurrently with migration.

And should be independently useful, whether or not part 2 is taken.

> 
> Signed-off-by: John Snow <address@hidden>
> ---

> +++ b/hw/misc/ivshmem.c
> @@ -740,7 +740,10 @@ static int pci_ivshmem_init(PCIDevice *dev)
>      if (s->role_val == IVSHMEM_PEER) {
>          error_setg(&s->migration_blocker,
>                     "Migration is disabled when using feature 'peer mode' in 
> device 'ivshmem'");
> -        migrate_add_blocker(s->migration_blocker);
> +        if (migrate_add_blocker(s->migration_blocker, NULL) < 0) {
> +            error_report("Unable to prohibit migration during ivshmem init");
> +            exit(1);
> +        }

Marc-André has been trying to get rid of exit(1) calls here, but this
matched the style at the time you wrote it.

> +++ b/hw/scsi/vhost-scsi.c
> @@ -239,8 +239,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error 
> **errp)
>                                 vhost_dummy_handle_output);
>      if (err != NULL) {
>          error_propagate(errp, err);
> -        close(vhostfd);
> -        return;
> +        goto close_fd;
> +    }
> +
> +    error_setg(&s->migration_blocker,
> +            "vhost-scsi does not support migration");

Indentation looks unusual...

> @@ -262,9 +268,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error 
> **errp)
>      /* Note: we can also get the minimum tpgt from kernel */
>      s->target = vs->conf.boot_tpgt;
>  
> -    error_setg(&s->migration_blocker,
> -            "vhost-scsi does not support migration");

...although it was merely preserved across code motion. You may still
want to fix it, though.

> +++ b/hw/virtio/vhost.c
> @@ -926,13 +926,25 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>          goto fail;
>      }
>  
> -    for (i = 0; i < hdev->nvqs; ++i) {
> -        r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
> -        if (r < 0) {
> -            goto fail_vq;
> -        }
> -    }
>      hdev->features = features;
> +    hdev->migration_blocker = NULL;
> +
> +    if (!(hdev->features & (0x1ULL << VHOST_F_LOG_ALL))) {
> +        error_setg(&hdev->migration_blocker,
> +                   "Migration disabled: vhost lacks VHOST_F_LOG_ALL 
> feature.");

error_setg() messages shouldn't end in '.', although this is once again
code motion. As before, you could fix it up.


> +++ b/stubs/migr-blocker.c
> @@ -1,8 +1,9 @@
>  #include "qemu-common.h"
>  #include "migration/migration.h"
>  
> -void migrate_add_blocker(Error *reason)
> +int migrate_add_blocker(Error *reason, Error **errp)
>  {
> +    return 0;

Should this set errp and return -EBUSY, so that callers don't assume
that their blocker is effective when you really ignored the blocker?
Then again, pre-patch, you silently ignored the blocker, so I guess it's
not much worse in semantics.

I spotted some minor things, but am comfortable with:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature