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

[Crístian Viana]

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.