[Kimchi-devel] [PATCH] issue #545: Handle simultaneous authentication methods when updating VM permission

[Aline Manera]

On 02/03/2015 16:19, Crístian Viana wrote:
> On 27-02-2015 13:34, Aline Manera wrote:
>>> +            same_auth = access_elem.xpath('./auth[@type="%s"]' % auth)
>>> +            if len(same_auth) > 0:
>>> +                # there is already a sub-element 'auth' with the 
>>> same type;
>>> +                # update it.
>>> +                auth_elem = same_auth[0]
>>> +
>>> +                if users is not None:
>>> +                    for u in auth_elem.findall('user'):
>>> +                        auth_elem.remove(u)
>>> +
>>> +                if groups is not None:
>>> +                    for g in auth_elem.findall('group'):
>>> +                        auth_elem.remove(g)
>>
>> I understood this block as a clean up - to remove old data and then 
>> insert the new one. Is that correct?
>
> Yes.
>
>> If so, I suggest to remove the whole "auth" element and create a new 
>> one below, as you already do.
>>
>>> +
>>> +        if auth_elem is None:
>>> +            # there is no sub-element 'auth' with the same type
>>> +            # (or no 'auth' at all); create it.
>>> +            auth_elem = E.auth(type=auth)
>>> +            access_elem.append(auth_elem)
>
> The cleanup code doesn't always remove all users and all groups from 
> "auth_elem"; it only removes the users if the request is setting new 
> users, and it only removes the groups if the request is settings new 
> groups. So there's an optimization in that code for when only one of 
> "users"/"groups" is set.
>
> Consider this example. Suppose the VM "foovm" has this XML data:
>
> [...]
> <auth type='pam'>
>   <user>u1</user>
>   <user>u2</user>
>   <group>g1</group
>   <group>g2</group>
> </auth>
> [...]
>
> and the following command is sent to the server:
>
> PUT /vms/foovm {'users': ['u10']}
>
> That command shouldn't change the current <group>s, only the <user>s 
> (because it didn't sent the params "groups"), so the server must keep 
> that VM's current <group>s settings. According to this patch, only 
> <user>s will be removed so the new values can be added later; <group>s 
> should remain. If the patch removes the "auth_elem" completely, as you 
> suggested, it will still need the old values from the XML data to set 
> the same groups again, and that will require more data/processing.
>
> In summary, this is the algorithm of this patch for the case described 
> above:
>
> 1) Read the XML from libvirt.
> 2) Remove the user data from the XML.
> 3) Add the new users to the XML, from params.
> 4) Send the new XML to libvirt.
>
> And this is the suggested algorithm:
>
> 1) Read the XML from libvirt.
> 2) Backup the group data.
> 3) Discard the XML.
> 4) Create an XML.
> 5) Add the new users to the XML, from params.
> 6) Add the old groups (from step (2)) to the XML.
> 7) Send the new XML to libvirt.
>
> Of course, when both "users" and "groups" are set in the same command, 
> these algorithms become equivalent. But it's always better to save 
> some time and memory when it's possible.

OK.