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 06:27:12PM +0100, Julien Grall wrote:
> On 24/08/2023 17:58, Anthony PERARD wrote:
> > On Thu, Aug 24, 2023 at 05:46:45PM +0100, Julien Grall wrote:
> > > 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?

It is to try to keep the code laid out in chronological order for this
async operation. See CODING_STYLE L153:
    
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libs/light/CODING_STYLE;h=b1a6a45c74df083cdd793e5cb6a67a76cb5c1174;hb=refs/heads/master#l153

(libxl_pci_assignable() isn't a good example, as it should be part of
the API ;-) )

Cheers,

-- 
Anthony PERARD