From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions Date: Sat, 02 May 2015 09:09:12 +0100 Message-ID: <55448628.2000200@citrix.com> References: <1430489141-27602-1-git-send-email-paul.durrant@citrix.com> <1430489141-27602-3-git-send-email-paul.durrant@citrix.com> <5543DFBC02000078000CE318@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YoSU9-0003sc-0m for xen-devel@lists.xenproject.org; Sat, 02 May 2015 08:09:17 +0000 In-Reply-To: <5543DFBC02000078000CE318@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , paul.durrant@citrix.com Cc: xen-devel@lists.xenproject.org, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 01/05/15 20:19, Jan Beulich wrote: >>>> Paul Durrant 05/01/15 4:05 PM >>> >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -5638,6 +5638,299 @@ static int hvmop_set_evtchn_upcall_vector( >> return 0; >> } > > >> +static int hvmop_set_param( >> + XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg) >> +{ >> + struct domain *curr_d = current->domain; >> + struct xen_hvm_param a; >> + struct domain *d; >> + struct vcpu *v; >> + int rc = 0; > Iirc Andrew indicated that Coverity would complain about dead initializers like > this. Likely, yes. It is less clear whether this case would trigger it, but I know for certain that it dislikes "int rc = 0; rc = fn();" > >> + if ( copy_from_guest(&a, arg, 1) ) >> + return -EFAULT; >> + >> + if ( a.index >= HVM_NR_PARAMS ) >> + return -EINVAL; >> + >> + d = rcu_lock_domain_by_any_id(a.domid); >> + if ( d == NULL ) >> + return -ESRCH; >> + >> + rc = -EINVAL; > (Not used anywhere up from here.) > >> + if ( is_pvh_domain(d) >> + && (a.index != HVM_PARAM_CALLBACK_IRQ) ) >> + goto out; > It would have been nice if you had corrected style issues like the misplaced && > as you go; I'll try to remember to do so while committing (together with a few more > and the adjustment for the issue above). > >> + case HVM_PARAM_IOREQ_PFN: >> + case HVM_PARAM_BUFIOREQ_PFN: >> + case HVM_PARAM_BUFIOREQ_EVTCHN: >> + { >> + domid_t domid; >> + >> + /* May need to create server */ >> + domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN]; >> + rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL); >> + if ( rc != 0 && rc != -EEXIST ) >> + goto out; >> + /*FALLTHRU*/ >> + } >> + default: > Andrew - will Coverity be happy with the fall-through comment being followed > by a closing brace? I don't have the exact reference to hand (I am still travelling), but I expect not. Such a brace could be part of an if/else clause which would mean that it wasn't the only codepath to fall through. ~Andrew > > Jan >