On Tue, Jan 26, 2021 at 03:38:27PM +0100, Jan Beulich wrote: > On 26.01.2021 12:06, Roger Pau Monne wrote: > > There are two duplicated calls to cleanup_domain_irq_pirq in > > unmap_domain_pirq. > > > > The first one in the for loop will be called with exactly the same > > arguments as the call placed closer to the loop start. > > I'm having trouble figuring out which two instances you refer > to: To me "first one" and "closer to the start" are two ways > of expressing the same thing. You don't refer to the call to > clear_domain_irq_pirq(), do you, when the two calls you > remove are to cleanup_domain_irq_pirq()? Admittedly quite > similar names, but entirely different functions.
Urg, yes, that's impossible to parse sensibly, sorry. Also the subject is wrong, should be cleanup_domain_irq_pirq, not clear_domain_irq_pirq. This should instead be: "There are two duplicated calls to cleanup_domain_irq_pirq in unmap_domain_pirq. The first removed call to cleanup_domain_irq_pirq will be called with exactly the same arguments as the previous call placed above it." It's hard to explain this in a commit message. Do you agree that the removed calls are duplicated though? I might have as well missed part of the logic here and be wrong about this. > > The logic used in the loop seems extremely complex to follow IMO, > > there are several breaks for exiting the loop, and the index (i) is > > also updated in different places. > > Indeed, and it didn't feel well already back at the time when > I much extended this to support multi-vector MSI. I simply > didn't have any good idea how to streamline all of this > (short of rewriting it altogether, which would have made > patch review quite difficult). If you have thoughts, I'm all > ears. I would just rewrite it altogether. Code like this is very prone to cause mistakes in the future IMO. If you want I can try to. I guess the problem with this is that we would lose the history of the code for no functional change. Thanks, Roger.
