Re: [PATCH v3] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics (and 1 more message)

From: Wei Liu <wei.liu2@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	IanJackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Daniel de Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [PATCH v3] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics (and 1 more message)
Date: Fri, 25 Aug 2017 17:05:44 +0100	[thread overview]
Message-ID: <20170825160544.ahurmrlk32lvuqec@citrix.com> (raw)
In-Reply-To: <59A0644A0200007800173F2D@prv-mh.provo.novell.com>

On Fri, Aug 25, 2017 at 09:54:18AM -0600, Jan Beulich wrote:
> >>> On 25.08.17 at 17:25, <wei.liu2@citrix.com> wrote:
> > On Wed, Aug 16, 2017 at 06:20:01AM -0600, Jan Beulich wrote:
> >> So far callers of the libxc interface passed in a domain ID which was
> >> then ignored in the hypervisor. Instead, make the hypervisor honor it
> >> (accepting DOMID_INVALID to obtain original behavior), allowing to
> >> query whether a device can be assigned to a particular domain.
> >> 
> >> Drop XSM's test_assign_{,dt}device hooks as no longer being
> >> individually useful.
> > 
> > Can you also say in the commit message that you consolidate some code as
> > well?
> 
> Am I consolidating code beyond what is reasonable to achieve
> the intended effect? I don't view the merging of the two case
> blocks 
> Oops, didn't finish here: "... as anything going beyond the main                                                                                              
> purpose of the patch. In fact if someone submitted a patch                                                                                                    
> without doing that folding, I'd ask for it to be done."  

It took more effort for reviewers to figure out the reason to delete
those two blocks just from looking at the diff, which distracted me a
bit. Of course I eventually figured out why they were deleted by looking
at the actual files, but had that been stated in commit message I could
have finished the review sooner because I would have a list of things to
look for in my mind and go through them faster.

Suppose you asked me to consolidate the blocks, I would happily do so
but also try to note that in the commit message, to help reviewer and my
future self to grasp what the patch does faster.

[...]
> >> +        }
> >> +
> > 
> > Move the ASSERT(d) here?
> 
> That would be a possibility, but personally I think it's better placed
> where it is now. It helps, for example, understanding why there is
> a NULL check of d somewhere in the middle. In a domctl handler d
> being NULL isn't a usual thing.
> 

Fair enough. I won't insist on moving it.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel