Re: [Xen-devel] [PATCH 09/19] xen: lock target domain in do_domctl common code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.11.12 at 17:44, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> On 11/20/2012 11:40 AM, Jan Beulich wrote:
>>>>> On 19.11.12 at 16:20, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
>>> On 11/19/2012 04:24 AM, Jan Beulich wrote:
>>>>>>> On 16.11.12 at 19:28, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
>>>>> @@ -458,6 +443,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>> u_domctl)
>>>>>          if ( IS_ERR(d) )
>>>>>          {
>>>>>              ret = PTR_ERR(d);
>>>>> +            d = NULL;
>>>>
>>>> Considering that in the common code you already set d to NULL,
>>>> is there a specific reason why you do so again here ...
>>>>
>>>>>              break;
>>>>>          }
>>>>>  
>>>>> @@ -469,39 +455,28 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>> u_domctl)
>>>>>          op->domain = d->domain_id;
>>>>>          if ( copy_to_guest(u_domctl, op, 1) )
>>>>>              ret = -EFAULT;
>>>>> +        d = NULL;
>>>>
>>>> ... and here?
>>>>
>>>> Same further down for XEN_DOMCTL_getdomaininfo.
>>>>
>>>> Jan
>>>>
>>>>>      }
>>>>>      break;
>>>>>  
>>>>
>>>>
>>>>
>>>
>>> This avoids unlocking the domain when it hasn't been locked (at the
>>> end of the function at domctl_out_unlock) or trying to unlock a
>>> ERR_PTR value.
>> 
>> Sorry, this doesn't explain why d needs to be set to NULL twice.
>> 
>> Jan
>> 
> 
> Maybe I misunderstood you: do you think one of the two assignments is
> redundant? They are not, since the above is immediately followed by a
> break, which jumps out of the switch statement to domctl_lock_release(),
> and does not hit the second assignment.

I thought they were redundant with the NULL assignment you added
close to the top of the function. But I realize I was wrong with that.

Jan


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