[libvirt] [PATCH v4 17/23] security_dac: Move transaction handling up one level

[Michal Privoznik]

On 09/17/2018 11:47 PM, John Ferlan wrote:
> 
> 
> On 09/10/2018 05:36 AM, Michal Privoznik wrote:
>> So far the whole transaction handling is done
>> virSecurityDACSetOwnershipInternal(). This needs to change for
>> the sake of security label remembering and locking. Otherwise we
>> would be locking a path when only appending it to transaction
>> list and not when actually relabelling it.
> 
> relabeling according to my spell checker...
> 
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/security/security_dac.c | 65 ++++++++++++++++++++++++++++++---------------
>>  1 file changed, 44 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 926c9a33c1..52e28b5fda 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
> 
> [...]
> 
>> @@ -201,11 +208,16 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>>          virSecurityDACChownItemPtr item = list->items[i];
>>  
>>          /* TODO Implement rollback */
>> -        if (virSecurityDACSetOwnershipInternal(list->priv,
>> -                                               item->src,
>> -                                               item->path,
>> -                                               item->uid,
>> -                                               item->gid) < 0)
>> +        if ((!item->restore &&
>> +             virSecurityDACSetOwnership(list->manager,
>> +                                        item->src,
>> +                                        item->path,
>> +                                        item->uid,
>> +                                        item->gid) < 0) ||
> 
> yuck - one of more least favorite constructs.
> 
>> +            (item->restore &&
>> +             virSecurityDACRestoreFileLabelInternal(list->manager,
>> +                                                    item->src,
>> +                                                    item->path) < 0))
>>              return -1;
>>      }
>>  
>> @@ -455,7 +467,6 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
>>  static int
>>  virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
>>  {
>> -    virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
>>      virSecurityDACChownListPtr list;
>>  
>>      list = virThreadLocalGet(&chownList);
>> @@ -468,7 +479,7 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
>>      if (VIR_ALLOC(list) < 0)
>>          return -1;
>>  
>> -    list->priv = priv;
>> +    list->manager = mgr;
> 
> Should this be a virObjectRef(mgr) with the corresponding virObjectUnref
> at the appropriate moment in time? I don't think there's a chance @mgr
> could be Unref'd otherwise, but every time I see this type construct for
> an object I want to be 'safe'.

I hear you, but I don't think that ref/unref is necessary here. It's
actually @mgr who called these driver APIs.

Michal