Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic

[Paolo Bonzini <pbonzini@redhat.com>]

Il 17/04/2013 17:59, Michael R. Hines ha scritto:
>>> Failure already happens for unknown capabilities.
>> Maybe I misread the code, but all I saw is:
>>
>> +        if (cap.flags & RDMA_CAPABILITY_CHUNK_REGISTER) {
>> +            rdma->chunk_register_destination = true;
>> +        } else if (cap.flags & RDMA_CAPABILITY_NEXT_FEATURE) {
>> +            /* handle new capability */
>> +        }
>>
>> and no failure.  I may well be wrong of course.
> 
> Sorry I miscommunicated. By "failure" I meant "disagreement".
> My design allows the two sides to "agree" with each other as I
> included in the documentation like this:
> 
> 1. User sets capability on source (or not)
> 
> 2. If 'true', source transmits our "intent" to activate capability to
> destination
> 
>           If (destination knows about the capability)
>                  then, enable on both sides
>           else
>                  disable on both sides
> 
> 3. If 'false', destination will never see the flag and will never know

Great.  That was my understanding too.

>>>> +            goto err_rdma_server_wait;
>>>> +    }
>>>> +
>>>> +    if (cap.version == RDMA_CONTROL_VERSION_1) {
>>>> +        if (cap.flags & RDMA_CAPABILITY_CHUNK_REGISTER) {
>>>> +            rdma->chunk_register_destination = true;
>>>> +        } else if (cap.flags & RDMA_CAPABILITY_NEXT_FEATURE) {
>>>> +            /* handle new capability */
>>>> +        }
>>>> As mentioned above, please drop this "else if".  But in general, this
>>>> "if" is useless.  Please replace it with an
>>>>
>>>>       /* We only support one version, and we rejected all
>>>>        * others above.
>>>>        */
>>>>       assert(cap.version == RDMA_CONTROL_VERSION_CURRENT);
>>> We don't want to kill QEMU with an assertion, do we?
>>> Shouldn't we throw the error back to the user?
>> You already filtered versions < min and > current with a decent error,
>> so the only reamining version at this point is 1.  You know cap.version
>> == RDMA_CONTROL_VERSION_CURRENT ("we rejected all others above").
> 
> So, as I described above, when version 2 comes out, both QEMU
> instances are able to "agree" only on the minimum capabilities
> that both sides support and still allow the migration to begin.
> 
> For that reason, I don't think we should be doing assert() here.
> We want this to be compatible across multiple QEMU versions.

The point of the assert is to ensure that all versions are either
handled correctly, or rejected.  It's to distinguish bugs from
unsupported versions.

    switch (cap.version) {
    case RDMA_CONTROL_VERSION_CURRENT:
        ...;
        break;
    default:
        assert(cap.version < RDMA_CONTROL_VERSION_MIN ||
               cap.version > RDMA_CONTROL_VERSION_CURRENT);
        /* give error */
    }

Then you can simply add new cases when new versions come out.

Paolo