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

[John Snow <jsnow@redhat.com>]

On 10/09/2015 03:42 PM, Eric Blake wrote:
> 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 <jsnow@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 <eblake@redhat.com>
> 

I'll tidy up the style issues on re-spin if I get a second review from
Kevin for the block portion and there's something else to fix up.

As for the migration blocker stub ... My assumption is that since we
were making no attempt to block migration, I didn't need to report some
kind of error. Does anyone else have a solid example of when this stub
gets used?

--js