Re: [PATCH] Convert backwards goto into a while loop

[Alex Williamson]

On Wed, 28 Dec 2022 21:32:12 +0000
Shachar Raindel <shacharr@xxxxxxxxxx> wrote:

> The function vaddr_get_pfns used a goto retry structure to implement
> retrying.  This is discouraged by the coding style guide (which is
> only recommending goto for handling function exits). Convert the code
> to a while loop, making it explicit that the follow block only runs
> when the pin attempt failed.

What coding style guide are you referring to?  In
Documentation/process/coding-style.rst I only see goto mentioned in 7)
Centralized exiting of functions, which suggests it's a useful
mechanism for creating centralized cleanup code, while noting "[a]lbeit
deprecated by *some people*", emphasis added.  This doesn't suggest to
me such a strong statement as implied in this commit log.
 
> Signed-off-by: Shachar Raindel <shacharr@xxxxxxxxxx>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c index 23c24fe98c00..7f38d7fc3f62
> 100644 --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -570,27 +570,28 @@ static int vaddr_get_pfns(struct mm_struct *mm,
> unsigned long vaddr, }
>  
>  		*pfn = page_to_pfn(pages[0]);
> -		goto done;
> -	}
> +	} else

Coding style would however discourage skipping the braces around this
half of the branch, as done here, as a) they're used around the former
half and b) the below is not a single simple statement.  Thanks,

Alex

> +		do {
> +
> +			/* This is not a normal page, lookup PFN for P2P DMA */
> +			vaddr = untagged_addr(vaddr);
>  
> -	vaddr = untagged_addr(vaddr);
> +			vma = vma_lookup(mm, vaddr);
>  
> -retry:
> -	vma = vma_lookup(mm, vaddr);
> +			if (!vma || !(vma->vm_flags & VM_PFNMAP))
> +				break;
>  
> -	if (vma && vma->vm_flags & VM_PFNMAP) {
> -		ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE);
> -		if (ret == -EAGAIN)
> -			goto retry;
> +			ret = follow_fault_pfn(vma, mm, vaddr, pfn,
> +					       prot & IOMMU_WRITE);
> +			if (ret)
> +				continue; /* Retry for EAGAIN, otherwise bail */
>  
> -		if (!ret) {
>  			if (is_invalid_reserved_pfn(*pfn))
>  				ret = 1;
>  			else
>  				ret = -EFAULT;
> -		}
> -	}
> -done:
> +		} while (ret == -EAGAIN);
> +
>  	mmap_read_unlock(mm);
>  	return ret;
>  }