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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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