qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 4/4] migration: Fail migration blocker for --


From: Greg Kurz
Subject: Re: [Qemu-devel] [PATCH v2 4/4] migration: Fail migration blocker for --only-migratble
Date: Fri, 16 Dec 2016 12:14:00 +0100

On Fri, 16 Dec 2016 15:23:44 +0530
Ashijeet Acharya <address@hidden> wrote:

> migrate_add_blocker should rightly fail if the '--only-migratable'
> option was specified and the device in use should not be able to
> perform the action which results in an unmigratable VM.
> 
> Make migrate_add_blocker return -EACCES in this case.
> 
> Signed-off-by: Ashijeet Acharya <address@hidden>
> ---
>  block/qcow.c                |  7 +++++--
>  block/vdi.c                 |  7 +++++--
>  block/vhdx.c                |  8 +++++---
>  block/vmdk.c                |  7 +++++--
>  block/vpc.c                 |  7 +++++--
>  block/vvfat.c               |  8 +++++---
>  hw/9pfs/9p.c                |  9 +++++++--
>  hw/display/virtio-gpu.c     |  9 +++++++--
>  hw/intc/arm_gic_kvm.c       |  7 +++++--
>  hw/intc/arm_gicv3_its_kvm.c |  5 ++++-
>  hw/intc/arm_gicv3_kvm.c     |  5 ++++-
>  hw/misc/ivshmem.c           | 10 ++++++++--
>  hw/scsi/vhost-scsi.c        | 10 ++++++----
>  hw/virtio/vhost.c           |  8 +++++---
>  migration/migration.c       |  7 +++++++
>  target-i386/kvm.c           |  7 +++++--
>  16 files changed, 88 insertions(+), 33 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 11526a1..7e5003ac 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -253,8 +253,11 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                 "does not support live migration",
>                 bdrv_get_device_or_node_name(bs));
>      ret = migrate_add_blocker(s->migration_blocker, errp);
> -    if (ret < 0) {
> -        error_free(s->migration_blocker);
> +    if (ret) {
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Cannot use a node with qcow format as "
> +                              "it does not support live migration");
> +        }
>          goto fail;
>      }
>  
> diff --git a/block/vdi.c b/block/vdi.c
> index 2b56f52..df45df4 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -472,8 +472,11 @@ static int vdi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                 "does not support live migration",
>                 bdrv_get_device_or_node_name(bs));
>      ret = migrate_add_blocker(s->migration_blocker, errp);
> -    if (ret < 0) {
> -        error_free(s->migration_blocker);
> +    if (ret) {
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Cannot use a node with vdi format as "
> +                              "it does not support live migration");
> +        }
>          goto fail_free_bmap;
>      }
>  
> diff --git a/block/vhdx.c b/block/vhdx.c
> index d81941b..118134e 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -996,11 +996,13 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                 "does not support live migration",
>                 bdrv_get_device_or_node_name(bs));
>      ret = migrate_add_blocker(s->migration_blocker, errp);
> -    if (ret < 0) {
> -        error_free(s->migration_blocker);
> +    if (ret) {
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Cannot use a node with vhdx format as "
> +                              "it does not support live migration");
> +        }
>          goto fail;
>      }
> -
>      if (flags & BDRV_O_RDWR) {
>          ret = vhdx_update_headers(bs, s, false, NULL);
>          if (ret < 0) {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 4a45a83..cd6a641 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -977,8 +977,11 @@ static int vmdk_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                 "does not support live migration",
>                 bdrv_get_device_or_node_name(bs));
>      ret = migrate_add_blocker(s->migration_blocker, errp);
> -    if (ret < 0) {
> -        error_free(s->migration_blocker);
> +    if (ret) {
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Cannot use a node with vmdk format as "
> +                              "it does not support live migration");
> +        }
>          goto fail;
>      }
>  
> diff --git a/block/vpc.c b/block/vpc.c
> index 299a8c8..c935962 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -427,8 +427,11 @@ static int vpc_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                 "does not support live migration",
>                 bdrv_get_device_or_node_name(bs));
>      ret = migrate_add_blocker(s->migration_blocker, errp);
> -    if (ret < 0) {
> -        error_free(s->migration_blocker);
> +    if (ret) {
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Cannot use a node with vpc format as "
> +                              "it does not support live migration");
> +        }
>          goto fail;
>      }
>  
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 3de3320..f9a5f9b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1192,8 +1192,11 @@ static int vvfat_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                     "does not support live migration",
>                     bdrv_get_device_or_node_name(bs));
>          ret = migrate_add_blocker(s->migration_blocker, errp);
> -        if (ret < 0) {
> -            error_free(s->migration_blocker);
> +        if (ret) {
> +            if (ret == -EACCES) {
> +                error_append_hint(errp, "Cannot use a node with vvfat format 
> "
> +                                  "as it does not support live migration");
> +            }
>              goto fail;
>          }
>      }
> @@ -1202,7 +1205,6 @@ static int vvfat_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          init_mbr(s, cyls, heads, secs);
>      }
>  
> -    //    assert(is_consistent(s));
>      qemu_co_mutex_init(&s->lock);
>  
>      ret = 0;
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index e72ef43..01dd67f 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1020,12 +1020,17 @@ static void coroutine_fn v9fs_attach(void *opaque)
>       */
>      if (!s->migration_blocker) {
>          int ret;
> +        Error *local_err;
>          s->root_fid = fid;
>          error_setg(&s->migration_blocker,
>                     "Migration is disabled when VirtFS export path '%s' is 
> mounted in the guest using mount_tag '%s'",
>                     s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
> -        ret = migrate_add_blocker(s->migration_blocker, NULL);
> -        if (ret < 0) {
> +        ret = migrate_add_blocker(s->migration_blocker, &local_err);
> +        if (ret) {

Why s/ret < 0/ret/ ? The migrate_add_blocker() function in this v2 only
returns 0 or a negative errno...

> +            if (ret == -EACCES) {
> +                error_append_hint(&local_err, "Failed to mount VirtFS as it "
> +                                  "does not support live migration");

And then local_err is leaked...

BTW, I'm not even sure if this useful since this is a guest originated action.

> +            }

This errno will be ultimately be returned to the mount(2) caller in the guest.

When reading the manpage, it looks like EBUSY definitely makes more sense than
EACCES:

       EACCES A component of a path was not searchable.  (See also  path_reso‐
              lution(7).)   Or,  mounting a read-only filesystem was attempted
              without giving the MS_RDONLY flag.  Or, the block device  source
              is located on a filesystem mounted with the MS_NODEV option.

       EBUSY  source  is  already  mounted.   Or, it cannot be remounted read-
              only, because it still holds files open  for  writing.   Or,  it
              cannot  be mounted on target because target is still busy (it is
              the working directory of some thread, the mount point of another
              device, has open files, etc.).

>              err = ret;
>              clunk_fid(s, fid);
>              goto out;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 6e60b8c..1b02f4b 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1102,6 +1102,7 @@ static void virtio_gpu_device_realize(DeviceState 
> *qdev, Error **errp)
>      VirtIOGPU *g = VIRTIO_GPU(qdev);
>      bool have_virgl;
>      int i;
> +    int ret;
>  
>      if (g->conf.max_outputs > VIRTIO_GPU_MAX_SCANOUTS) {
>          error_setg(errp, "invalid max_outputs > %d", 
> VIRTIO_GPU_MAX_SCANOUTS);
> @@ -1120,8 +1121,12 @@ static void virtio_gpu_device_realize(DeviceState 
> *qdev, Error **errp)
>  
>      if (virtio_gpu_virgl_enabled(g->conf)) {
>          error_setg(&g->migration_blocker, "virgl is not yet migratable");
> -        if (migrate_add_blocker(g->migration_blocker, errp) < 0) {
> -            error_free(g->migration_blocker);
> +        ret = migrate_add_blocker(g->migration_blocker, errp);
> +        if (ret) {
> +            if (ret == -EACCES) {
> +                error_append_hint(errp, "Cannot use virgl as it does not "
> +                                  "support live migration yet");
> +            }
>              return;
>          }
>      }
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 3614daa..b40ad5f 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -514,8 +514,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error 
> **errp)
>          error_setg(&s->migration_blocker, "This operating system kernel does 
> "
>                                            "not support vGICv2 migration");
>          ret = migrate_add_blocker(s->migration_blocker, errp);
> -        if (ret < 0) {
> -            error_free(s->migration_blocker);
> +        if (ret) {
> +            if (ret == -EACCES) {
> +                error_append_hint(errp, "Failed to realize vGICv2 as its"
> +                                  " migrataion is not implemented yet");
> +            }
>              return;
>          }
>      }
> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index 950a02f..3474642 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -70,7 +70,10 @@ static void kvm_arm_its_realize(DeviceState *dev, Error 
> **errp)
>      error_setg(&s->migration_blocker, "vITS migration is not implemented");
>      ret = migrate_add_blocker(s->migration_blocker, errp);
>      if (ret < 0) {
> -        error_free(s->migration_blocker);
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Failed to realize vITS as its migration 
> "
> +                              "is not implemented yet");
> +        }
>          return;
>      }
>  
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 06f6f30..be7b97c 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -118,7 +118,10 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, 
> Error **errp)
>      error_setg(&s->migration_blocker, "vGICv3 migration is not implemented");
>      ret = migrate_add_blocker(s->migration_blocker, errp);
>      if (ret < 0) {
> -        error_free(s->migration_blocker);
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Failed to realize vGICv3 as its 
> migration"
> +                              "is not implemented yet");
> +        }
>          return;
>      }
>  
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 585cc5b..58f27b1 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -837,6 +837,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error 
> **errp)
>  {
>      IVShmemState *s = IVSHMEM_COMMON(dev);
>      Error *err = NULL;
> +    int ret;
>      uint8_t *pci_conf;
>      uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>          PCI_BASE_ADDRESS_MEM_PREFETCH;
> @@ -910,8 +911,13 @@ static void ivshmem_common_realize(PCIDevice *dev, Error 
> **errp)
>      if (!ivshmem_is_master(s)) {
>          error_setg(&s->migration_blocker,
>                     "Migration is disabled when using feature 'peer mode' in 
> device 'ivshmem'");
> -        if (migrate_add_blocker(s->migration_blocker, errp) < 0) {
> -            error_free(s->migration_blocker);
> +        ret = migrate_add_blocker(s->migration_blocker, errp);
> +        if (ret) {
> +            if (ret == -EACCES) {
> +                error_append_hint(errp, "Cannot use the 'peer mode' feature "
> +                                  "in device 'ivshmem' as it does not "
> +                                  "support live migration");
> +            }
>              return;
>          }
>      }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index ff503d0..0c571f8 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -244,8 +244,12 @@ static void vhost_scsi_realize(DeviceState *dev, Error 
> **errp)
>      error_setg(&s->migration_blocker,
>                 "vhost-scsi does not support migration");
>      ret = migrate_add_blocker(s->migration_blocker, errp);
> -    if (ret < 0) {
> -        goto free_blocker;
> +    if (ret) {
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Cannot use vhost-scsi as it does not "
> +                              "support live migration");
> +        }
> +        goto close_fd;
>      }
>  
>      s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
> @@ -272,8 +276,6 @@ static void vhost_scsi_realize(DeviceState *dev, Error 
> **errp)
>   free_vqs:
>      migrate_del_blocker(s->migration_blocker);
>      g_free(s->dev.vqs);
> - free_blocker:
> -    error_free(s->migration_blocker);
>   close_fd:
>      close(vhostfd);
>      return;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 416fada..1c4a4f9 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1159,9 +1159,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void 
> *opaque,
>      if (hdev->migration_blocker != NULL) {
>          Error *local_err;
>          r = migrate_add_blocker(hdev->migration_blocker, &local_err);
> -        if (r < 0) {
> -            error_report_err(local_err);
> -            error_free(hdev->migration_blocker);
> +        if (r) {
> +            if (r == -EACCES) {
> +                error_append_hint(&local_err, "Cannot use vhost drivers as "
> +                                  "it does not support live migration");
> +            }
>              goto fail_busyloop;
>          }
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index 713e012..ab34328 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1115,9 +1115,16 @@ int migrate_add_blocker(Error *reason, Error **errp)
>  {
>      if (migration_is_idle(NULL)) {
>          migration_blockers = g_slist_prepend(migration_blockers, reason);
> +        error_free(reason);
>          return 0;
>      }
>  
> +    if (only_migratable) {
> +        error_setg(errp, "Failed to add migration blocker: --only-migratable 
> "
> +                   "was specified");
> +        return -EACCES;
> +    }
> +
>      error_setg(errp, "Cannot block a migration already in-progress");
>      return -EBUSY;
>  }
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 6031a1c..05c8365 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -962,7 +962,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>                     "State blocked by non-migratable CPU device"
>                     " (invtsc flag)");
>          r = migrate_add_blocker(invtsc_mig_blocker, NULL);
> -        if (r < 0) {
> +        if (r) {
> +            if (r == -EACCES) {
> +                error_report("kvm: non-migratable CPU device but"
> +                        " --only-migratable was specified");
> +            }
>              error_report("kvm: migrate_add_blocker failed");
>              goto efail;
>          }
> @@ -1009,7 +1013,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>   fail:
>      migrate_del_blocker(invtsc_mig_blocker);
>   efail:
> -    error_free(invtsc_mig_blocker);
>      return r;
>  }
>  




reply via email to

[Prev in Thread] Current Thread [Next in Thread]