Re: [Qemu-block] [Qemu-devel] [RFC v4 15/21] blockjobs: add prepare callback

[John Snow]

On 02/27/2018 02:56 PM, Eric Blake wrote:
> On 02/23/2018 05:51 PM, John Snow wrote:
>> Some jobs upon finalization may need to perform some work that can
>> still fail. If these jobs are part of a transaction, it's important
>> that these callbacks fail the entire transaction.
>>
>> We allow for a new callback in addition to commit/abort/clean that
>> allows us the opportunity to have fairly late-breaking failures
>> in the transactional process.
>>
>> The expected flow is:
>>
>> - All jobs in a transaction converge to the WAITING state
>>    (added in a forthcoming commit)
>> - All jobs prepare to call either commit/abort
>> - If any job fails, is canceled, or fails preparation, all jobs
>>    call their .abort callback.
>> - All jobs enter the PENDING state, awaiting manual intervention
>>    (also added in a forthcoming commit)
>> - block-job-finalize is issued by the user/management layer
>> - All jobs call their commit callbacks.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   blockjob.c                   | 34 +++++++++++++++++++++++++++++++---
>>   include/block/blockjob_int.h | 10 ++++++++++
>>   2 files changed, 41 insertions(+), 3 deletions(-)
>>
> 
>> @@ -467,17 +480,22 @@ static void block_job_cancel_async(BlockJob *job)
>>       job->cancelled = true;
>>   }
>>   -static void block_job_txn_apply(BlockJobTxn *txn, void fn(BlockJob *))
>> +static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *))
>>   {
>>       AioContext *ctx;
>>       BlockJob *job, *next;
>> +    int rc;
>>         QLIST_FOREACH_SAFE(job, &txn->jobs, txn_list, next) {
>>           ctx = blk_get_aio_context(job->blk);
>>           aio_context_acquire(ctx);
>> -        fn(job);
>> +        rc = fn(job);
>>           aio_context_release(ctx);
>> +        if (rc) {
>> +            break;
>> +        }
> 
> This short-circuits the application of the function to the rest of the
> group.  Is that ever going to be a problem?
> 

With what I've written, I don't think so -- but I can't guarantee
someone won't misunderstand the semantics of it and it will become a
problem. It is a potentially dangerous function in that way.

--js