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

[Julien Grall <julien@xxxxxxx>]

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.
> > > If it does needs to be a separate function, a better way to lay it down
> > > would be to replace calls to pci_remove_detached() by
> > > pci_revoke_permissions() as appropriate, and rename it with the prefixed
> > > "pci_remove_", that is pci_remove_revoke_permissions().
> > I don't understand this suggestion. pci_revoke_permissions() is called right
> > in the middle of pci_remove_detached(). So it is not clear how it can be
> > called ahead.
> >
> > Also, if I replace pci_remove_detached() with pci_revoke_permissions(), does
> > this mean you are expecting the latter to call the former?
> Let's just keep things simpler, and just add the code into
> pci_remove_detached().
I will attempt to fold the code. But I am not convinced about the 
simplicity and readability.
Cheers,

--
Julien Grall