Re: [Xen-devel] [PATCH v2 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/05/15 20:19, Jan Beulich wrote:
> > > > Paul Durrant <paul.durrant@xxxxxxxxxx> 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

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