qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 3/3] migration: disallow migrate_add_blocker dur


From: Ashijeet Acharya
Subject: Re: [Qemu-devel] [PATCH 3/3] migration: disallow migrate_add_blocker during migration
Date: Thu, 15 Dec 2016 23:42:16 +0530

On Thu, Dec 15, 2016 at 11:21 PM, John Snow <address@hidden> wrote:
>
>
> On 12/15/2016 12:11 PM, Dr. David Alan Gilbert wrote:
>>      if (virtio_gpu_virgl_enabled(g->conf)) {
>> +        error_setg(&g->migration_blocker, "virgl is not yet migratable");
>> +        ret = migrate_add_blocker(g->migration_blocker, errp);
>> +        if (ret) {
>> +            if (ret > 0) {
>> +                error_setg(errp, "Cannot use virgl as it does not support 
>> live"
>> +                           " migration yet and --only-migratable was "
>> +                           "specified");
>> +            }
>> +            return;
>> +        }
>> +    }
>
> It may be best to leave this patch as a generic "Failed to add a
> migration blocker" type of patch.
>
> Then, in a fourth patch, you add the --only-migratable specific stuff as
> an enhancement. (Basically, add your changes on top of my patch in your
> own, separate patch.)

No problem, I will separate them. Although I will add changes for the
ARM drivers you first missed out on.

>
> I'd also add the "--only-migratable" stuff only inside
> migrate_add_blocker, and whatever the reason happens to be, you can
> append a hint to the error message in the caller above.
>
> e.g.
>
> migrate_add_blocker(..) {
> ..
>   error_setg("Failed to add migration blocker: --only-migratable was
> specified")
> ..
> }
>
> And in the caller:
>
> r = migrate_add_blocker(.., errp)
> if (r) {
>   error_append_hint(errp, "Failed to initialize virgl, couldn't add
> migration blocker")
> }
>
> Or something along those lines. Maybe even error_prepend in this case
> makes sense.

Yes, I will make this change and make the error message more like:
error_append_hint(errp, "Failed to initialize virgl as it does not
support live migration, couldn't add migration blocker");

>
> I'd try to keep all of the --only-migratable logic localized and in a
> separate patch, leaving this patch strictly to deal with the case where
> a migration is already in progress. Of course, you'll be right to notice
> that in many of these initialization cases the error path could never
> trigger, but that's OK. It adds the generic handling necessary to cope
> with an error from a lower layer.
>
> I'd also certainly advise to never return a custom error code from
> migrate_add_blocker if we also wish to return a 'real' error code. Stick
> with POSIX entirely or avoid it entirely.

Right, as discussed with Dave on the IRC, I will make use of EACCES.

Ashijeet
>
> --js



reply via email to

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