lkml.org 
[lkml]   [2022]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()
Date


> On Feb 24, 2022, at 2:03 AM, Dan Carpenter <dan.carpenter@oracle.com> 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@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.

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!
\
 
 \ /
  Last update: 2022-02-24 17:10    [W:0.331 / U:0.372 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site