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

18 Sep 2018

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@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

    