Re: [PATCH 2/2] tools/light: Revoke permissions when a PCI detach for HVM domain

[Julien Grall <julien@xxxxxxx>]

On 24/08/2023 17:58, Anthony PERARD wrote:
> On Thu, Aug 24, 2023 at 05:46:45PM +0100, Julien Grall wrote:
> > Hi Anthony,
> >
> > On 24/08/2023 17:34, Anthony PERARD wrote:
> > > On Thu, Aug 24, 2023 at 12:15:39PM +0100, Julien Grall wrote:
> > > > On 18/08/2023 18:04, Anthony PERARD wrote:
> > > > > So, this new pci_revoke_permissions() function been place before
> > > > > do_pci_remove() will make it harder to follow what do_pci_remove() does.
> > > > > Does it need to be a separate function? Can't you inline it in
> > > > > pci_remove_detached() ?
> > > > I decided to go with an inline function to avoid increasing the size of
> > > > pci_remove_detached() and also separate the logic from cleaning-up QMP an
> > > > resetting the PCI device.
> > > It's fine to have a long function, if there's that much to do. You can
> > > add a comment if you want to separate a bit from the rest.
> > >
> > > Having a new function would be ok if it was used from different places,
> > > or if it was needed for a new callback in the chain of callbacks of a
> > > long-running operation.
> > I don't agree with this definition on when to create a new function. Smaller
> > functions help to logically split your code and also avoids to have to
> > define 20 local variables right at the beginning of the function (this is
> > what will happen with folding the function) among other advantages.
> You can always create a new block {} in the function, if that help with
> local variables.
I thought about it... But this is just a function in disguised with 
downside (the extra layer of indentation).
I was actually going to try the folding version. But given this 
suggestion, I am now struggling to understand why this is a problem to 
have a function only called once. This is fairly common in Xen 
Hypervisor and I can see at least one instance in libxl (see 
libxl_pci_assignable()). So what is the rationale behind this request?
I would also like the opinion from others (such as Juergen) before going 
ahead with any changes.
Cheers,

--
Julien Grall