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

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(a)redhat.com&gt; > --- > 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'.