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

[Dan Carpenter <dan.carpenter@oracle.com>]

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@wanadoo.fr> 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.

regards,
dan carpenter