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 <js...@redhat.com>
> ---
> +++ 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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
