Re: [libvirt] [PATCH v4 05/23] qemu_security: Run transactions more frequently

On 09/10/2018 05:36 AM, Michal Privoznik wrote: Perhaps the first sentence or so "differently said": Now that committing transactions using pid == -1 means that we're not fork()-ing to run the transaction in a specific namespace, we can utilize the transaction processing semantics in order to start, run a or multiple commands, and then commit the transaction without being concerned with other interactions or transactions interrupting the processing. This will eventually allow us to have a single place where all the paths... (as you've already written). This patch made Coverity unhappy because some of the void callers to virSecurityManagerTransactionStart or virSecurityManagerTransactionCommit don't check status: qemuSecurityStartTPMEmulator (cleanup: processing) qemuSecurityCleanupTPMEmulator qemuSecurityRestoreAllLabel I know existing, if a transaction fails to start does it need to be aborted... I know it doesn't matter by looking at the code, but that thought could be important later on. Interesting that in this instance you choose to honor the status, but for another void function qemuSecurityCleanupTPMEmulator the choice was to not care. IDC which way this is done, but it should be consistent. I can also see a reason that if a TransactionStart fails, that we at least attempt the following rather than just returning. In that case, I'd say to use the < 0 status as a way to not call TransactionCommit and perhaps throw a VIR_WARN to indicate that transactions weren't being used. Probably also want to save/restore LastError around all this so that a failure in TransactionStart or TransactionCommit doesn't overwrite an error we already have. If we wrap this in ignore_value, then Coverity will be "happy". Since the only consumer is ProcessStop, at least we don't have the fear that a Start succeeds and a Commit fails that we'll be "stuck" the next time we go to do a Start... I guess to some degree I wonder if the transaction even matters - meaning - I think you can stick with the original code so that Start/Commit failure doesn't mean anything. I also think this function should be separated from the others since its adding transaction semantics where they weren't present previously. That way we can debate need or decide later to revert if necessary... [...] I almost want to say this is "different" than any of the other functions which you are converting the existing transaction model. In this instance you are introducing a transaction model. I think this should be a separate patch to make it easier for a later decision to revert just in case that decision needs to be made. The only current consumer is virSecuritySELinuxSetTPMLabels and it already has the multiple operation type semantics, so I'm wondering if transaction semantics are actually necessary. We should change this to "error:" because it's not cleanup in normal path... I think this should be: if (!transactionStarted) { if (virSecurityManagerTransactionStart(...) == 0) transactionStarted = true; } Making the subsequent be: if (transactionStarted) { ignore_value(virSecurityManagerTransactionCommit(...); virSecurityManagerTransactionAbort(...); } Because if the "second" Start fails, then next ones aren't necessary. I'd also be inclined to say, why bother with a transaction. If something in the Set processing failed, I'd assume we're going to get a similar failure in Restore processing - so let the TPM specific code handle that. We can document the crap out of it^w^w^w^w, err, ah decision made. So this is different than qemuSecurityRestoreAllLabel... Similar to above - we should know if Start fails, because if it does, then commit and abort won't be happy either... This should be a separate patch with the StartTPMEmulator... > + > virSecurityManagerRestoreTPMLabels(driver->securityManager, def); > + > + virSecurityManagerTransactionCommit(driver->securityManager, -1); > + virSecurityManagerTransactionAbort(driver->securityManager); [...] So, let's do this for any function that is not adding transaction semantics, but merely modifying existing semantics, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; But for qemuSecurityRestoreAllLabel, qemuSecurityStartTPMEmulator, and qemuSecurityCleanupTPMEmulator - we need to extract those and consider them separately. Whether you alter the commit message to my suggestion above is your choice. John