On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote: > On 23/06/16 14:25, Marek Marczykowski-Górecki wrote: > > On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote: > >>>>> On 23.06.16 at 11:23, wrote: > >>> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote: > >>>> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: > >>>>>>>> On 23.06.16 at 10:57, wrote: > >>>>>> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: > >>>>>>> I wonder what good the duplication of the returned domain ID does: I'm > >>>>>>> tempted to remove the one in the command-specific structure. Does > >>>>>>> anyone have insight into why it was done that way? > >>>>>> Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first > >>>>>> existing domain with ID >= passed one? Reading various comments in code > >>>>>> it looks to be used to domain enumeration. This patch changes this > >>>>>> behaviour. > >>>>> No, it doesn't. It adjusts the behavior only for the DM case (which > >>>>> isn't supposed to get information on other than the target domain, > >>>>> i.e. in this one specific case the very domain ID needs to be passed > >>>>> in). > >>>> int xc_domain_getinfo(xc_interface *xch, > >>>> uint32_t first_domid, > >>>> unsigned int max_doms, > >>>> xc_dominfo_t *info) > >>>> { > >>>> unsigned int nr_doms; > >>>> uint32_t next_domid = first_domid; > >>>> DECLARE_DOMCTL; > >>>> int rc = 0; > >>>> > >>>> memset(info, 0, max_doms*sizeof(xc_dominfo_t)); > >>>> > >>>> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) > >>>> { > >>>> domctl.cmd = XEN_DOMCTL_getdomaininfo; > >>>> domctl.domain = (domid_t)next_domid; > >>>> if ( (rc = do_domctl(xch, &domctl)) < 0 ) > >>>> break; > >>>> info->domid = (uint16_t)domctl.domain; > >>>> (...) > >>>> next_domid = (uint16_t)domctl.domain + 1; > >>>> > >>>> > >>>> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next > >>> valid > >>>> domain. > >>> Hmm, looks like I've misread you patch. Reading again... > >>> > >>> But now I see rcu_read_lock(&domlist_read_lock) is gets called only when > >>> looping over domains, but rcu_read_unlock is called in any case. Is it > >>> correct? > >> How that? There is this third hunk: > > Ok, after drawing a flowchart of the control in this function after your > > change, on a piece of paper, this case looks fine. But depending on how > > the domain was found (explicit loop or rcu_lock_domain_by_id), different > > locks are held, which makes it harder to follow what is going on. > > > > Crazy idea: how about making the code easy/easier to read instead of > > obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is > > convolved enough. How about this version (2 patches): > > > > xen: move domain lookup for getdomaininfo to the same > > > > XEN_DOMCTL_getdomaininfo have different semantics than most of others > > domctls - it returns information about first valid domain with ID >= > > argument. But that's no excuse for having the lookup done in a different > > place, which made handling different corner cases unnecessary complex. > > Move the lookup to the first switch clause. And adjust locking to be the > > same as for other cases. > > > > Signed-off-by: Marek Marczykowski-Górecki > > FWIW, I prefer this solution to the issue. > > Reviewed-by: Andrew Cooper , with a few style > nits. Fixed patch according to your comments: xen: move domain lookup for getdomaininfo to the same XEN_DOMCTL_getdomaininfo have different semantics than most of others domctls - it returns information about first valid domain with ID >= argument. But that's no excuse for having the lookup code in a different place, which made handling different corner cases unnecessary complex. Move the lookup to the first switch clause. And adjust locking to be the same as for other cases. Signed-off-by: Marek Marczykowski-Górecki --- xen/common/domctl.c | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/xen/common/domctl.c b/xen/common/domctl.c index e43904e..41de3e8 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -442,11 +442,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) switch ( op->cmd ) { case XEN_DOMCTL_createdomain: - case XEN_DOMCTL_getdomaininfo: case XEN_DOMCTL_test_assign_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; + + case XEN_DOMCTL_getdomaininfo: + d = rcu_lock_domain_by_id(op->domain); + + if ( d == NULL ) + { + /* Search for the next available domain. */ + rcu_read_lock(&domlist_read_lock); + + for_each_domain ( d ) + if ( d->domain_id >= op->domain ) + { + rcu_lock_domain(d); + break; + } + + rcu_read_unlock(&domlist_read_lock); + if ( d == NULL ) + return -ESRCH; + } + break; + default: d = rcu_lock_domain_by_id(op->domain); if ( d == NULL ) @@ -862,33 +883,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) break; case XEN_DOMCTL_getdomaininfo: - { - domid_t dom = op->domain; - - rcu_read_lock(&domlist_read_lock); - - for_each_domain ( d ) - if ( d->domain_id >= dom ) - break; - - ret = -ESRCH; - if ( d == NULL ) - goto getdomaininfo_out; - ret = xsm_getdomaininfo(XSM_HOOK, d); if ( ret ) - goto getdomaininfo_out; + break; getdomaininfo(d, &op->u.getdomaininfo); - op->domain = op->u.getdomaininfo.domain; copyback = 1; - - getdomaininfo_out: - rcu_read_unlock(&domlist_read_lock); - d = NULL; break; - } case XEN_DOMCTL_getvcpucontext: { -- 2.5.5 -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?