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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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.

Cheers,

-- 
Anthony PERARD