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

[Ashijeet Acharya]

On Fri, Dec 16, 2016 at 4:07 PM, Greg Kurz <address@hidden> wrote:
> On Fri, 16 Dec 2016 15:23:43 +0530
> Ashijeet Acharya <address@hidden> 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.
>>
>> Signed-off-by: John Snow <address@hidden>
>> Signed-off-by: Ashijeet Acharya <address@hidden>
>> ---
>>  block/qcow.c                  |  6 +++++-
>>  block/vdi.c                   |  6 +++++-
>>  block/vhdx.c                  | 16 ++++++++++------
>>  block/vmdk.c                  |  7 ++++++-
>>  block/vpc.c                   | 10 +++++++---
>>  block/vvfat.c                 | 20 ++++++++++++--------
>>  hw/9pfs/9p.c                  | 16 ++++++++++++----
>>  hw/display/virtio-gpu.c       | 29 ++++++++++++++++-------------
>>  hw/intc/arm_gic_kvm.c         | 16 ++++++++++------
>>  hw/intc/arm_gicv3_its_kvm.c   | 18 +++++++++++-------
>>  hw/intc/arm_gicv3_kvm.c       | 19 ++++++++++++-------
>>  hw/misc/ivshmem.c             | 11 +++++++----
>>  hw/scsi/vhost-scsi.c          | 25 +++++++++++++++++++------
>>  hw/virtio/vhost.c             |  8 +++++++-
>>  include/migration/migration.h |  6 +++++-
>>  migration/migration.c         | 35 +++++++++++++++++++++++++++++++++--
>>  stubs/migr-blocker.c          |  3 ++-
>>  target-i386/kvm.c             | 16 +++++++++++++---
>>  18 files changed, 192 insertions(+), 75 deletions(-)
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index faebd91..e72ef43 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque)
>>          goto out;
>>      }
>>      err += offset;
>> -    memcpy(&s->root_qid, &qid, sizeof(qid));
>> -    trace_v9fs_attach_return(pdu->tag, pdu->id,
>> -                             qid.type, qid.version, qid.path);
>> +
>>      /*
>>       * disable migration if we haven't done already.
>>       * attach could get called multiple times for the same export.
>>       */
>>      if (!s->migration_blocker) {
>> +        int ret;
>>          s->root_fid = fid;
>
> Since we may fail now, I guess ^^ should move...
>
>>          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);
>> -        migrate_add_blocker(s->migration_blocker);
>> +        ret = migrate_add_blocker(s->migration_blocker, NULL);
>> +        if (ret < 0) {
>> +            err = ret;
>> +            clunk_fid(s, fid);
>
> Please don't rely on put_fid() to rollback what was done in this
> function. You need to do the following here:
> - error_free(s->migration_blocker)
> - s->migration_blocker = NULL

Okay, I was not aware of this. I have added these two now.

>
>> +            goto out;
>> +        }
>
>           s->root_fid = fid;
>
> ... here.

Right. I missed out on this one. I have moved it now.

Ashijeet
>
>>      }
>> +
>> +    memcpy(&s->root_qid, &qid, sizeof(qid));
>> +    trace_v9fs_attach_return(pdu->tag, pdu->id,
>> +                             qid.type, qid.version, qid.path);
>>  out:
>>      put_fid(pdu, fidp);
>>  out_nofid: