On Thu, Dec 15, 2016 at 11:21 PM, John Snow <js...@redhat.com> 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
