On Thu, 16 Feb 2023 17:52:33 +0100
Juan Quintela <quint...@redhat.com> wrote:
> 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:-)
This is not a huge amount of data on the stack, so I'm fine with it as
is. Thanks,
Alex
