Re: [PATCH v11 08/11] vfio/migration: Implement VFIO migration protocol v2

[Juan Quintela]

Avihai Horon <avihaih@nvidia.com> wrote:
> On 16/02/2023 17:43, Juan Quintela wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Avihai Horon <avihaih@nvidia.com> wrote:
>>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>.
>>
>>
>> 1st comment is a bug, but as you just remove it.
>>
>>
>> Not that it matters a lot in the end (you are removing v1 on the
>> following patch).
>>
>>> @@ -438,7 +445,13 @@ static bool 
>>> vfio_devices_all_running_and_mig_active(VFIOContainer *container)
>>>                   return false;
>>>               }
>>>
>>> -            if (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) 
>>> {
>>> +            if (!migration->v2 &&
>>> +                migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) 
>>> {
>>> +                continue;
>>> +            }
>> Are you missing here:
>>
>>
>>                 } else {
>>                     return false;
>>                 }
>>
>> Or I am missunderstanding the code.
>
> I think the code is OK:
>
> If the device uses v1 migration and is running, the first if is true
> and we continue.
> If the device uses v1 migration and is not running, the first if is
> false and the second if is false (since the device doesn't use v2
> migration) and we return false.
>
> If the device uses v2 migration and is running, the first if is false
> and the second if is true, so we continue.
> If the device uses v2 migration and is not running, first if is false
> and the second if is false, so we return false.

You win O:-)

I was looking at C level, not at the "semantic" level.

>>
>>         size_t size = DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
>>                                   sizeof(struct 
>> vfio_device_feature_mig_data_size),
>>                                   sizeof(uint64_t));
>>         g_autofree struct vfio_device_feature *feature = g_malloc0(size * 
>> sizeof(uint64_t));
>>
>> I have to reread several times to see what was going on there.
>>
>>
>> And call it a day?
>
> I don't have a strong opinion here.
> We can do whatever you/Alex like more.

I think it is more readable, and we don't put (normally) big chunks in
the stack, but once told that, who wrotes the code has more to say O:-)

Later, Juan.