Avihai Horon <avih...@nvidia.com> wrote:
> On 16/02/2023 17:43, Juan Quintela wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Avihai Horon <avih...@nvidia.com> wrote:
>>
>> Reviewed-by: Juan Quintela <quint...@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.
