Re: [Xen-devel] [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Aug 24, 2017 at 9:24 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 24.08.17 at 17:17, <aisaila@xxxxxxxxxxxxxxx> wrote:
>> On Jo, 2017-08-24 at 07:24 -0600, Jan Beulich wrote:
>>> > @@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct
>>> > vm_event_domain *ved)
>>> >  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain
>>> > *ved,
>>> >                            bool_t allow_sleep)
>>> >  {
>>> > +    if ( !ved )
>>> > +        return -ENOSYS;
>>> I don't think ENOSYS is correct here.
>> Can you tell me what is the preferred return value here?
>
> -EOPNOTSUPP is what we commonly use in such cases.
>
>>> > @@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d,
>>> > xen_domctl_vm_event_op_t *vec,
>>> >  #ifdef CONFIG_HAS_MEM_PAGING
>>> >      case XEN_DOMCTL_VM_EVENT_OP_PAGING:
>>> >      {
>>> > -        struct vm_event_domain *ved = &d->vm_event->paging;
>>> Dropping this local variable (and similar ones below) pointlessly
>>> increases the size of this patch.
>> I have dropped the local var because in case of a XEN_VM_EVENT_ENABLE
>> d->vm_event_... is allocated in the vm_event_enable function below so
>> it will allocate mem for the local variable.
>
> I don't see how that renders the local variable useless. But anyway,
> its the maintainers of that code to judge.

I guess the reason for dropping it is that vm_event_enable will place
the location of the structure into the pointer that was passed in, so
if you are passing in a pointer that was locally declared you would
then still have to copy it back to d->vm_event_ which doesn't make
much sense. IMHO it's fine how it's changed in the patch.

Tamas

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