Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Vishnu Dasa]

> On Feb 24, 2022, at 2:03 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 
> On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote:
>> 
>>> On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
>>> 
>>> Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
>>>> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
> 
>>>> Whatever...  But where this really
>>>> hurts is with:
>>>> Alloc:
>>>> 	if (vmci_dev->exclusive_vectors) {
>>>> 		error = request_irq(pci_irq_vector(pdev, 1), ...
>>>> Free:
>>>> 	free_irq(pci_irq_vector(pdev, 1), vmci_dev);
>>>> No if statement.  It works because it's the last allocation but it's
>>>> confusing and fragile.
>>> 
>>> Agreed.
>> 
>> Sorry, I hope I'm not missing something obvious or misunderstanding the point.
>> But I don't think the problem implied here exists?
>> 
>> If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
>> is no free_irq in this path.  If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
>> goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'.  Note that
>> this is for the previous one without the check for vmci_dev->exclusive_vectors.
> 
> It's not a bug, but it's bad style.
> 
> Ideally the allocation code and the free code should mirror each other
> as much as possible.  If there is an if statement in the allocation code
> we should copy that same if statement into the free code.  Even if we
> can leave the if statement out, we should still include it for
> readability.
> 
> Also if we add another allocation at the end of the function then we
> will have to add the if statement back.  Maybe the person who adds the
> if statement will forget.  So it's fragile.

No disagreements about that.  Making sure I wasn't missing anything
else.

Could we have a separate fix for this which fixes?
"VMCI: dma dg: register dummy IRQ handlers for DMA datagrams"

Thanks!