Re: [PATCH V2 2/8] vfio/pci: Remove negative check on unsigned vector

[Reinette Chatre]

Hi Alex,

On 3/30/2023 3:54 PM, Alex Williamson wrote:
> On Thu, 30 Mar 2023 15:32:20 -0700
> Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
>> 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);
> 

Much better. Will do. Thank you very much.

Reinette