On Thu, 30 Mar 2023 15:32:20 -0700
Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
> Hi Alex,
>
> On 3/30/2023 1:26 PM, Alex Williamson wrote:
> > On Tue, 28 Mar 2023 14:53:29 -0700
> > Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
> ...
>
> >> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> >> static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
> >> unsigned count, int32_t *fds, bool msix)
> >> {
> >> - int i, j, ret = 0;
> >> + int i, ret = 0;
> >> + unsigned int j;
> >>
> >> if (start >= vdev->num_ctx || start + count > vdev->num_ctx)
> >> return -EINVAL;
> >
> > Unfortunately this turns the unwind portion of the function into an
> > infinite loop in the common case when @start is zero:
> >
> > for (--j; j >= (int)start; j--)
> > vfio_msi_set_vector_signal(vdev, j, -1, msix);
> >
> >
>
> Thank you very much for catching this. It is not clear to me how you
> would prefer to resolve this. Would you prefer that the vector parameter
> in vfio_msi_set_vector_signal() continue to be an int and this patch be
> dropped and the "if (vector < 0)" check remains (option A)? Or, alternatively,
> I see two other possible solutions where the vector parameter in
> vfio_msi_set_vector_signal() becomes an unsigned int and the above snippet
> could be one of:
>
> option B:
> vfio_msi_set_block()
> {
> int i, j, ret = 0;
>
> ...
> for (--j; j >= (int)start; j--)
> vfio_msi_set_vector_signal(vdev, (unsigned int)j, -1, msix);
> }
>
> option C:
> vfio_msi_set_block()
> {
> int i, ret = 0;
> unsigned int j;
>
> ...
> for (--j; j >= start && j < start + count; j--)
> vfio_msi_set_vector_signal(vdev, j, -1, msix);
> }
>
> What would you prefer?
Hmm, C is fine, it avoids casting. I think we could also do:
unsigned int i, j;
int ret = 0;
...
for (i = start; i < j; i++)
vfio_msi_set_vector_signal(vdev, i, -1, msix);
Thanks,
Alex
