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 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.

> > 
> > 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().

Cheers,

-- 
Anthony PERARD