From: Ondřej Surý Date: Mon, 13 Jul 2026 09:42:10 +0000 (+0200) Subject: Pass the work callback result to the done callback X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=66392c034e0f3ccb573eb5ab44328945a22a1be3;p=thirdparty%2Fbind9.git Pass the work callback result to the done callback The isc_work callback returned void, so every user that cared about the outcome of the offloaded work had to smuggle it through its own context state (xfrin_work_t, the catz/rpz updateresult fields, result members in the dump/load/checksig contexts). Make the work callback return isc_result_t and have isc_work deliver that value to the done callback. --- diff --git a/bin/dig/nslookup.c b/bin/dig/nslookup.c index ac7a6c8285a..46e99391196 100644 --- a/bin/dig/nslookup.c +++ b/bin/dig/nslookup.c @@ -760,7 +760,7 @@ do_next_command(char *input) { } } -static void +static isc_result_t readline_next_command(void *arg) { char *ptr = NULL; @@ -770,7 +770,7 @@ readline_next_command(void *arg) { ptr = readline("> "); isc_loopmgr_nonblocking(); if (ptr == NULL) { - return; + return ISC_R_SUCCESS; } if (*ptr != 0) { @@ -779,13 +779,17 @@ readline_next_command(void *arg) { cmdline = cmdlinebuf; } free(ptr); + + return ISC_R_SUCCESS; } -static void +static isc_result_t fgets_next_command(void *arg) { UNUSED(arg); cmdline = fgets(cmdlinebuf, COMMSIZE, stdin); + + return ISC_R_SUCCESS; } ISC_NORETURN static void diff --git a/lib/dns/catz.c b/lib/dns/catz.c index b63a4251267..d9316fb71d3 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -92,7 +92,6 @@ struct dns_catz_zone { bool updatepending; /* there is an update pending */ bool updaterunning; /* there is an update running */ - isc_result_t updateresult; /* result from the offloaded work */ dns_db_t *db; /* zones database */ dns_dbversion_t *dbversion; /* version we will be updating to */ dns_db_t *updb; /* zones database we're working on */ @@ -114,7 +113,7 @@ dns__catz_timer_start(dns_catz_zone_t *catz); static void dns__catz_timer_stop(void *arg); -static void +static isc_result_t dns__catz_update_cb(void *data); static void dns__catz_done_cb(void *data, isc_result_t result); @@ -2033,7 +2032,6 @@ dns__catz_timer_cb(void *arg) { catz->updatepending = false; catz->updaterunning = true; - catz->updateresult = ISC_R_UNSET; dns_name_format(&catz->name, domain, DNS_NAME_FORMATSIZE); @@ -2043,7 +2041,6 @@ dns__catz_timer_cb(void *arg) { "catz: %s: no longer active, reload is canceled", domain); catz->updaterunning = false; - catz->updateresult = ISC_R_CANCELED; goto exit; } @@ -2159,7 +2156,7 @@ catz_rdatatype_is_processable(const dns_rdatatype_t type) { * It creates a new catz, iterates over database to fill it with content, and * then merges new catz into old catz. */ -static void +static isc_result_t dns__catz_update_cb(void *data) { dns_catz_zone_t *catz = (dns_catz_zone_t *)data; dns_db_t *updb = NULL; @@ -2189,8 +2186,7 @@ dns__catz_update_cb(void *data) { catzs = catz->catzs; if (atomic_load(&catzs->shuttingdown)) { - result = ISC_R_SHUTTINGDOWN; - goto exit; + return ISC_R_SHUTTINGDOWN; } dns_name_format(&updb->origin, bname, DNS_NAME_FORMATSIZE); @@ -2202,8 +2198,7 @@ dns__catz_update_cb(void *data) { LOCK(&catzs->lock); if (catzs->zones == NULL) { UNLOCK(&catzs->lock); - result = ISC_R_SHUTTINGDOWN; - goto exit; + return ISC_R_SHUTTINGDOWN; } result = isc_ht_find(catzs->zones, r.base, r.length, (void **)&oldcatz); is_active = (result == ISC_R_SUCCESS && oldcatz->active); @@ -2213,7 +2208,7 @@ dns__catz_update_cb(void *data) { isc_log_write(DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_CATZ, ISC_LOG_ERROR, "catz: zone '%s' not in config", bname); - goto exit; + return result; } if (!is_active) { @@ -2221,8 +2216,7 @@ dns__catz_update_cb(void *data) { isc_log_write(DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_CATZ, ISC_LOG_INFO, "catz: zone '%s' is no longer active", bname); - result = ISC_R_CANCELED; - goto exit; + return ISC_R_CANCELED; } result = dns_db_getsoaserial(updb, oldcatz->updbversion, &vers); @@ -2232,7 +2226,7 @@ dns__catz_update_cb(void *data) { ISC_LOG_ERROR, "catz: zone '%s' has no SOA record (%s)", bname, isc_result_totext(result)); - goto exit; + return result; } isc_log_write(DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_CATZ, ISC_LOG_INFO, @@ -2245,7 +2239,7 @@ dns__catz_update_cb(void *data) { ISC_LOG_ERROR, "catz: failed to create DB iterator - %s", isc_result_totext(result)); - goto exit; + return result; } name = dns_fixedname_initname(&fixname); @@ -2262,7 +2256,7 @@ dns__catz_update_cb(void *data) { ISC_LOG_ERROR, "catz: failed to create name from string - %s", isc_result_totext(result)); - goto exit; + return result; } result = dns_dbiterator_seek(updbit, name); @@ -2273,7 +2267,8 @@ dns__catz_update_cb(void *data) { "catz: zone '%s' has no 'version' record (%s) " "and will not be processed", bname, isc_result_totext(result)); - goto exit; + + return result; } newcatz = dns_catz_zone_new(catzs, &updb->origin); @@ -2417,8 +2412,7 @@ dns__catz_update_cb(void *data) { "will not be processed", bname); dns_catz_zone_detach(&newcatz); - result = ISC_R_FAILURE; - goto exit; + return ISC_R_FAILURE; } /* @@ -2431,19 +2425,18 @@ dns__catz_update_cb(void *data) { ISC_LOG_ERROR, "catz: failed merging zones: %s", isc_result_totext(result)); - goto exit; + return result; } isc_log_write(DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_CATZ, ISC_LOG_DEBUG(3), "catz: update_from_db: new zone merged"); -exit: - catz->updateresult = result; + return result; } static void -dns__catz_done_cb(void *data, isc_result_t result ISC_ATTR_UNUSED) { +dns__catz_done_cb(void *data, isc_result_t result) { dns_catz_zone_t *catz = (dns_catz_zone_t *)data; char dname[DNS_NAME_FORMATSIZE]; @@ -2466,7 +2459,7 @@ dns__catz_done_cb(void *data, isc_result_t result ISC_ATTR_UNUSED) { isc_log_write(DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_CATZ, ISC_LOG_INFO, "catz: %s: reload done: %s", dname, - isc_result_totext(catz->updateresult)); + isc_result_totext(result)); dns_catz_zone_unref(catz); } diff --git a/lib/dns/include/dns/rpz.h b/lib/dns/include/dns/rpz.h index 8dac8746877..4904a470da4 100644 --- a/lib/dns/include/dns/rpz.h +++ b/lib/dns/include/dns/rpz.h @@ -156,7 +156,6 @@ struct dns_rpz_zone { bool dbregistered; /* db callback notify is registered. */ bool updatepending; /* there is an update pending */ bool updaterunning; /* there is an update running */ - isc_result_t updateresult; /* result from the offloaded work */ dns_db_t *db; /* zones database */ dns_dbversion_t *dbversion; /* version we will be updating to */ dns_db_t *updb; /* zones database we're working on */ diff --git a/lib/dns/master.c b/lib/dns/master.c index f380a69b976..b70fb4d99d3 100644 --- a/lib/dns/master.c +++ b/lib/dns/master.c @@ -2569,37 +2569,40 @@ cleanup: } /* - * The combination of isc_work_enqueue() on the current loop and callback on - * lctx->loop ensures the correct ordering: + * The load runs on the SLOW work lane of lctx->loop: * - * 1. dns_master_loadfileasync() calls isc_work_enqueue() on the current loop. - * 2. master_load() runs asynchronously and can finish before the entry point - * returns; master_load_done() is queued on the current loop and cannot run - * until the entry point returns. - * 3. The entry point publishes *lctxp. - * 4. master_load_done() runs on the current loop and hands off to lctx->loop. - * 5. lctx->done() runs on lctx->loop asynchronously. + * 1. dns_master_loadfileasync() publishes *lctxp, then starts + * master_load_start() on lctx->loop with isc_async_run(). Publishing + * first ensures lctx->done() cannot observe *lctxp unset even when the + * caller runs on a different loop. + * 2. master_load_start() enqueues the work; isc_work_enqueue() must be + * called on the loop the work is bound to, hence the extra hop. + * 3. master_load() runs on the worker thread and its result is handed to + * master_load_done() on lctx->loop. + * 4. master_load_done() calls lctx->done() and drops the loop and lctx + * references. */ -static void +static isc_result_t master_load(void *arg) { dns_loadctx_t *lctx = arg; - lctx->result = (lctx->load)(lctx); + return (lctx->load)(lctx); } static void -master_load_callback(void *arg) { +master_load_done(void *arg, isc_result_t result) { dns_loadctx_t *lctx = arg; - (lctx->done)(lctx->done_arg, lctx->result); + (lctx->done)(lctx->done_arg, result); isc_loop_detach(&lctx->loop); dns_loadctx_detach(&lctx); } static void -master_load_done(void *arg, isc_result_t result ISC_ATTR_UNUSED) { +master_load_start(void *arg) { dns_loadctx_t *lctx = arg; - isc_async_run(lctx->loop, master_load_callback, lctx); + isc_work_enqueue(lctx->loop, ISC_WORKLANE_SLOW, master_load, + master_load_done, lctx); } isc_result_t @@ -2632,11 +2635,12 @@ dns_master_loadfileasync(const char *master_file, dns_name_t *top, dns_loadctx_ref(lctx); isc_loop_attach(loop, &lctx->loop); - isc_work_enqueue(isc_loop(), ISC_WORKLANE_SLOW, master_load, - master_load_done, lctx); + /* Publish *lctxp before the load can start (see master_load). */ *lctxp = lctx; + isc_async_run(loop, master_load_start, lctx); + return ISC_R_SUCCESS; } diff --git a/lib/dns/masterdump.c b/lib/dns/masterdump.c index af077a86667..2040a832d03 100644 --- a/lib/dns/masterdump.c +++ b/lib/dns/masterdump.c @@ -247,7 +247,6 @@ struct dns_dumpctx { dns_dumpdonefunc_t done; void *done_arg; /* dns_master_dumpasync() */ - isc_result_t result; char *file; char *tmpfile; dns_masterformat_t format; @@ -1451,19 +1450,20 @@ closeandrename(FILE *f, isc_result_t result, const char *temp, } /* - * The combination of isc_work_enqueue() on the current loop and callback on - * dctx->loop ensures the correct ordering: + * The dump runs on the SLOW work lane of dctx->loop: * - * 1. dns_master_dumptostreamasync() (or dns_master_dumpasync()) calls - * isc_work_enqueue() on the current loop. - * 2. master_dump() runs asynchronously and can finish before the entry point - * returns; master_dump_done() is queued on the current loop and cannot run - * until the entry point returns. - * 3. The entry point publishes *dctxp. - * 4. master_dump_done() runs on the current loop and hands off to dctx->loop. - * 5. dctx->done() runs on dctx->loop asynchronously. + * 1. dns_master_dumptostreamasync() (or dns_master_dumpasync()) publishes + * *dctxp, then starts master_dump_start() on dctx->loop with + * isc_async_run(). Publishing first ensures dctx->done() cannot observe + * *dctxp unset even when the caller runs on a different loop. + * 2. master_dump_start() enqueues the work; isc_work_enqueue() must be + * called on the loop the work is bound to, hence the extra hop. + * 3. master_dump() runs on the worker thread and its result is handed to + * master_dump_done() on dctx->loop. + * 4. master_dump_done() calls dctx->done() and drops the loop and dctx + * references. */ -static void +static isc_result_t master_dump(void *data) { isc_result_t result = ISC_R_UNSET; dns_dumpctx_t *dctx = data; @@ -1486,23 +1486,24 @@ master_dump(void *data) { result = flushandsync(dctx->f, result, NULL); } - dctx->result = result; + return result; } static void -master_dump_callback(void *data) { +master_dump_done(void *data, isc_result_t result) { dns_dumpctx_t *dctx = data; - (dctx->done)(dctx->done_arg, dctx->result); + (dctx->done)(dctx->done_arg, result); isc_loop_detach(&dctx->loop); dns_dumpctx_detach(&dctx); } static void -master_dump_done(void *data, isc_result_t result ISC_ATTR_UNUSED) { +master_dump_start(void *data) { dns_dumpctx_t *dctx = data; - isc_async_run(dctx->loop, master_dump_callback, dctx); + isc_work_enqueue(dctx->loop, ISC_WORKLANE_SLOW, master_dump, + master_dump_done, dctx); } static isc_result_t @@ -1742,11 +1743,12 @@ dns_master_dumptostreamasync(isc_mem_t *mctx, dns_db_t *db, dns_dumpctx_ref(dctx); isc_loop_attach(loop, &dctx->loop); - isc_work_enqueue(isc_loop(), ISC_WORKLANE_SLOW, master_dump, - master_dump_done, dctx); + /* Publish *dctxp before the dump can start (see master_dump). */ *dctxp = dctx; + isc_async_run(loop, master_dump_start, dctx); + return ISC_R_SUCCESS; } @@ -1835,11 +1837,12 @@ dns_master_dumpasync(isc_mem_t *mctx, dns_db_t *db, dns_dbversion_t *version, dns_dumpctx_ref(dctx); isc_loop_attach(loop, &dctx->loop); - isc_work_enqueue(isc_loop(), ISC_WORKLANE_SLOW, master_dump, - master_dump_done, dctx); + /* Publish *dctxp before the dump can start (see master_dump). */ *dctxp = dctx; + isc_async_run(loop, master_dump_start, dctx); + return ISC_R_SUCCESS; cleanup_tempname: diff --git a/lib/dns/message.c b/lib/dns/message.c index 0f7e6733af5..94618bb4743 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -199,7 +199,6 @@ typedef struct checksig_ctx { dns_view_t *view; dns_message_cb_t cb; void *cbarg; - isc_result_t result; } checksig_ctx_t; /* @@ -3003,20 +3002,19 @@ dns_message_dumpsig(dns_message_t *msg, char *txt1) { static void checksig_done(void *arg, isc_result_t result); -static void +static isc_result_t checksig_run(void *arg) { checksig_ctx_t *chsigctx = arg; - chsigctx->result = dns_message_checksig(chsigctx->msg, chsigctx->view); + return dns_message_checksig(chsigctx->msg, chsigctx->view); } static void -checksig_done(void *arg, isc_result_t result ISC_ATTR_UNUSED) { +checksig_done(void *arg, isc_result_t result) { checksig_ctx_t *chsigctx = arg; dns_message_t *msg = chsigctx->msg; - chsigctx->cb(chsigctx->cbarg, - (result != ISC_R_SUCCESS) ? result : chsigctx->result); + chsigctx->cb(chsigctx->cbarg, result); dns_view_detach(&chsigctx->view); isc_loop_detach(&chsigctx->loop); @@ -3036,7 +3034,6 @@ dns_message_checksig_async(dns_message_t *msg, dns_view_t *view, *chsigctx = (checksig_ctx_t){ .cb = cb, .cbarg = cbarg, - .result = ISC_R_UNSET, .loop = isc_loop_ref(loop), }; dns_message_attach(msg, &chsigctx->msg); diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index a4b13460430..a0c6737e4fa 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -1661,7 +1661,7 @@ dns__rpz_timer_stop(void *arg) { } static void -update_rpz_done_cb(void *data, isc_result_t result ISC_ATTR_UNUSED) { +update_rpz_done_cb(void *data, isc_result_t result) { dns_rpz_zone_t *rpz = (dns_rpz_zone_t *)data; char dname[DNS_NAME_FORMATSIZE]; @@ -1689,7 +1689,7 @@ update_rpz_done_cb(void *data, isc_result_t result ISC_ATTR_UNUSED) { isc_log_write(DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_RPZ, ISC_LOG_INFO, "rpz: %s: reload done: %s", dname, - isc_result_totext(rpz->updateresult)); + isc_result_totext(result)); dns_rpz_zones_unref(rpz->rpzs); } @@ -1899,7 +1899,7 @@ dns__rpz_shuttingdown(dns_rpz_zones_t *rpzs) { return ISC_R_SUCCESS; } -static void +static isc_result_t update_rpz_cb(void *data) { dns_rpz_zone_t *rpz = (dns_rpz_zone_t *)data; isc_result_t result = ISC_R_SUCCESS; @@ -1907,10 +1907,7 @@ update_rpz_cb(void *data) { REQUIRE(rpz->nodes != NULL); - result = dns__rpz_shuttingdown(rpz->rpzs); - if (result != ISC_R_SUCCESS) { - goto shuttingdown; - } + RETERR(dns__rpz_shuttingdown(rpz->rpzs)); isc_ht_init(&newnodes, rpz->rpzs->mctx, 1, ISC_HT_CASE_SENSITIVE); @@ -1924,8 +1921,7 @@ update_rpz_cb(void *data) { cleanup: isc_ht_destroy(&newnodes); -shuttingdown: - rpz->updateresult = result; + return result; } static void @@ -1946,7 +1942,6 @@ dns__rpz_timer_cb(void *arg) { rpz->updatepending = false; rpz->updaterunning = true; - rpz->updateresult = ISC_R_UNSET; dns_db_attach(rpz->db, &rpz->updb); INSIST(rpz->dbversion != NULL); diff --git a/lib/dns/validator.c b/lib/dns/validator.c index cba632f6e39..d7136309edc 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -134,7 +134,7 @@ validate_async_done(dns_validator_t *val, isc_result_t result); static isc_result_t validate_async_run(dns_validator_t *val, isc_job_cb cb); static isc_result_t -validate_work_enqueue(dns_validator_t *val, isc_job_cb cb); +validate_work_enqueue(dns_validator_t *val, isc_work_cb cb); static void validate_dnskey(void *arg); @@ -410,7 +410,7 @@ over_max_fails(dns_validator_t *val); static void consume_validation_fail(dns_validator_t *val); -static void +static isc_result_t resume_answer_with_key(void *arg) { dns_validator_t *val = arg; dns_rdataset_t *rdataset = &val->frdataset; @@ -427,7 +427,7 @@ resume_answer_with_key(void *arg) { consume_validation_fail(val); } - (void)validate_async_run(val, resume_answer_with_key_done); + return validate_async_run(val, resume_answer_with_key_done); } static void @@ -1740,7 +1740,7 @@ validate_answer_finish(void *arg); static void validate_answer_signing_key_done(void *arg); -static void +static isc_result_t validate_answer_signing_key(void *arg) { dns_validator_t *val = arg; isc_result_t result; @@ -1781,7 +1781,7 @@ validate_answer_signing_key(void *arg) { break; } - (void)validate_async_run(val, validate_answer_signing_key_done); + return validate_async_run(val, validate_answer_signing_key_done); } static void @@ -1991,7 +1991,7 @@ null_done(void *arg ISC_ATTR_UNUSED, isc_result_t result ISC_ATTR_UNUSED) { } static isc_result_t -validate_work_enqueue(dns_validator_t *val, isc_job_cb cb) { +validate_work_enqueue(dns_validator_t *val, isc_work_cb cb) { val->attributes |= VALATTR_OFFLOADED; isc_work_enqueue(val->loop, ISC_WORKLANE_FAST, cb, null_done, val); return DNS_R_WAIT; @@ -2292,7 +2292,7 @@ validate_dnskey_dsset(dns_validator_t *val) { static void validate_dnskey_dsset_next_done(void *arg); -static void +static isc_result_t validate_dnskey_dsset_next(void *arg) { dns_validator_t *val = arg; @@ -2307,7 +2307,7 @@ validate_dnskey_dsset_next(void *arg) { val->result = validate_dnskey_dsset(val); } - validate_async_run(val, validate_dnskey_dsset_next_done); + return validate_async_run(val, validate_dnskey_dsset_next_done); } static void diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c index 408dc707e9c..bf5a3a71a6b 100644 --- a/lib/dns/xfrin.c +++ b/lib/dns/xfrin.c @@ -200,15 +200,6 @@ struct dns_xfrin { #define XFRIN_MAGIC ISC_MAGIC('X', 'f', 'r', 'I') #define VALID_XFRIN(x) ISC_MAGIC_VALID(x, XFRIN_MAGIC) -#define XFRIN_WORK_MAGIC ISC_MAGIC('X', 'f', 'r', 'W') -#define VALID_XFRIN_WORK(x) ISC_MAGIC_VALID(x, XFRIN_WORK_MAGIC) - -typedef struct xfrin_work { - unsigned int magic; - isc_result_t result; - dns_xfrin_t *xfr; -} xfrin_work_t; - /**************************************************************************/ /* * Forward declarations. @@ -309,7 +300,7 @@ cleanup: return result; } -static void +static isc_result_t axfr_apply(void *arg); static isc_result_t @@ -325,13 +316,7 @@ axfr_putdata(dns_xfrin_t *xfr, dns_diffop_t op, dns_name_t *name, dns_ttl_t ttl, CHECK(dns_zone_checknames(xfr->zone, name, rdata)); if (dns_diff_size(&xfr->diff) > 128) { - xfrin_work_t work = (xfrin_work_t){ - .magic = XFRIN_WORK_MAGIC, - .result = ISC_R_UNSET, - .xfr = xfr, - }; - axfr_apply((void *)&work); - CHECK(work.result); + CHECK(axfr_apply(xfr)); } dns_difftuple_create(xfr->diff.mctx, op, name, ttl, rdata, &tuple); @@ -345,12 +330,9 @@ cleanup: /* * Store a set of AXFR RRs in the database. */ -static void +static isc_result_t axfr_apply(void *arg) { - xfrin_work_t *work = arg; - REQUIRE(VALID_XFRIN_WORK(work)); - - dns_xfrin_t *xfr = work->xfr; + dns_xfrin_t *xfr = arg; REQUIRE(VALID_XFRIN(xfr)); isc_result_t result = ISC_R_SUCCESS; @@ -370,18 +352,15 @@ axfr_apply(void *arg) { cleanup: dns_diff_clear(&xfr->diff); - work->result = result; + + return result; } static void -axfr_apply_done(void *arg, isc_result_t eresult) { - xfrin_work_t *work = arg; - dns_xfrin_t *xfr = work->xfr; - isc_result_t result = (eresult == ISC_R_SUCCESS) ? work->result - : eresult; +axfr_apply_done(void *arg, isc_result_t result) { + dns_xfrin_t *xfr = arg; REQUIRE(VALID_XFRIN(xfr)); - REQUIRE(VALID_XFRIN_WORK(work)); if (atomic_load(&xfr->shuttingdown)) { result = ISC_R_SHUTTINGDOWN; @@ -398,8 +377,6 @@ axfr_apply_done(void *arg, isc_result_t eresult) { cleanup: xfr->diff_running = false; - isc_mem_put(xfr->mctx, work, sizeof(*work)); - if (result == ISC_R_SUCCESS) { if (atomic_load(&xfr->state) == XFRST_AXFR_END) { xfrin_end(xfr, result); @@ -415,15 +392,10 @@ static void axfr_commit(dns_xfrin_t *xfr) { REQUIRE(!xfr->diff_running); - xfrin_work_t *work = isc_mem_get(xfr->mctx, sizeof(*work)); - *work = (xfrin_work_t){ - .magic = XFRIN_WORK_MAGIC, - .result = ISC_R_UNSET, - .xfr = dns_xfrin_ref(xfr), - }; + dns_xfrin_ref(xfr); xfr->diff_running = true; isc_work_enqueue(xfr->loop, ISC_WORKLANE_SLOW, axfr_apply, - axfr_apply_done, work); + axfr_apply_done, xfr); } static isc_result_t @@ -572,14 +544,12 @@ cleanup: return result; } -static void +static isc_result_t ixfr_apply(void *arg) { - xfrin_work_t *work = arg; - dns_xfrin_t *xfr = work->xfr; + dns_xfrin_t *xfr = arg; isc_result_t result = ISC_R_SUCCESS; REQUIRE(VALID_XFRIN(xfr)); - REQUIRE(VALID_XFRIN_WORK(work)); struct __cds_wfcq_head diff_head; struct cds_wfcq_tail diff_tail; @@ -611,20 +581,14 @@ ixfr_apply(void *arg) { isc_mem_put(xfr->mctx, data, sizeof(*data)); } - work->result = result; + return result; } static void -ixfr_apply_done(void *arg, isc_result_t eresult) { - xfrin_work_t *work = arg; - REQUIRE(VALID_XFRIN_WORK(work)); - - dns_xfrin_t *xfr = work->xfr; +ixfr_apply_done(void *arg, isc_result_t result) { + dns_xfrin_t *xfr = arg; REQUIRE(VALID_XFRIN(xfr)); - isc_result_t result = (eresult == ISC_R_SUCCESS) ? work->result - : eresult; - if (atomic_load(&xfr->shuttingdown)) { result = ISC_R_SHUTTINGDOWN; } @@ -636,15 +600,13 @@ ixfr_apply_done(void *arg, isc_result_t eresult) { !cds_wfcq_empty(&xfr->diff_head, &xfr->diff_tail)) { isc_work_enqueue(xfr->loop, ISC_WORKLANE_SLOW, ixfr_apply, - ixfr_apply_done, work); + ixfr_apply_done, xfr); return; } cleanup: xfr->diff_running = false; - isc_mem_put(xfr->mctx, work, sizeof(*work)); - /* * Don't retry with AXFR (even if it was requested) because there was * an error or the transfer is shutting down. In case if it _was_ an @@ -709,15 +671,10 @@ ixfr_commit(dns_xfrin_t *xfr) { &data->wfcq_node); if (!xfr->diff_running) { - xfrin_work_t *work = isc_mem_get(xfr->mctx, sizeof(*work)); - *work = (xfrin_work_t){ - .magic = XFRIN_WORK_MAGIC, - .result = ISC_R_UNSET, - .xfr = dns_xfrin_ref(xfr), - }; + dns_xfrin_ref(xfr); xfr->diff_running = true; isc_work_enqueue(xfr->loop, ISC_WORKLANE_SLOW, ixfr_apply, - ixfr_apply_done, work); + ixfr_apply_done, xfr); } cleanup: diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index 465f29da9e4..50eb6519fcd 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -741,7 +741,7 @@ httpd_compress(isc_httpd_sendreq_t *req) { } #endif /* ifdef HAVE_ZLIB */ -static void +static isc_result_t prepare_response(void *arg) { isc_httpd_sendreq_t *req = arg; isc_httpd_t *httpd = req->httpd; @@ -866,6 +866,8 @@ prepare_response(void *arg) { } httpd->recvlen -= httpd->consume; httpd->consume = 0; + + return ISC_R_SUCCESS; } static void diff --git a/lib/isc/include/isc/work.h b/lib/isc/include/isc/work.h index 8cb0c3f6d2f..9f4cf4c9f59 100644 --- a/lib/isc/include/isc/work.h +++ b/lib/isc/include/isc/work.h @@ -16,9 +16,9 @@ * * Each isc event loop has one worker thread per lane (see isc_worklane_t). * isc_work_enqueue() runs a callback on the worker thread bound to the calling - * loop's lane and, when it finishes, runs a second callback back on that loop. - * The handle it returns can be used to cancel a task that has not started - * running yet. + * loop's lane and, when it finishes, runs a second callback back on that loop + * with the first callback's result. The handle it returns can be used to + * cancel a task that has not started running yet. */ #pragma once @@ -36,7 +36,7 @@ typedef enum isc_worklane { * holding up short FAST tasks behind it. */ -typedef void (*isc_work_cb)(void *arg); +typedef isc_result_t (*isc_work_cb)(void *arg); typedef void (*isc_work_done_cb)(void *arg, isc_result_t result); typedef struct isc_work isc_work_t; @@ -46,18 +46,18 @@ isc_work_enqueue(isc_loop_t *loop, isc_worklane_t lane, isc_work_cb cb, /*%< * Schedule 'cb' to run on the worker thread bound to 'loop' and 'lane'. When * 'cb' returns, 'done_cb' is scheduled back on 'loop' with the result of the - * work: ISC_R_SUCCESS normally, or ISC_R_CANCELED if the task was canceled + * work: the value 'cb' returned, or ISC_R_CANCELED if the task was canceled * before it started (see isc_work_cancel()). * * Returns a handle that may be passed to isc_work_cancel(). The handle is - * owned by 'loop' and stays valid until 'after_cb' has run; it must not be used + * owned by 'loop' and stays valid until 'done_cb' has run; it must not be used * afterwards. * * Requires: * *\li 'loop' is a valid isc event loop. *\li 'cb' is non-NULL. - *\li 'after_cb' is non-NULL. + *\li 'done_cb' is non-NULL. *\li 'cbarg' is passed to both callbacks, may be NULL. */ @@ -66,7 +66,7 @@ isc_work_cancel(isc_work_t *work); /*%< * Try to cancel 'work' before its 'cb' starts running. If the task is still * queued it is marked canceled and 'cb' will not run; if it is already running - * or has finished this has no effect. Either way the 'after_cb' passed to + * or has finished this has no effect. Either way the 'done_cb' passed to * isc_work_enqueue() still runs on the origin loop, with ISC_R_CANCELED when * the cancel succeeded. Nothing is freed here. * @@ -77,7 +77,7 @@ isc_work_cancel(isc_work_t *work); * * Requires: * - *\li 'work' is a handle from isc_work_enqueue() whose 'after_cb' has not run. + *\li 'work' is a handle from isc_work_enqueue() whose 'done_cb' has not run. */ /* private */ diff --git a/lib/isc/work.c b/lib/isc/work.c index 69c751cf101..fc0e09d945b 100644 --- a/lib/isc/work.c +++ b/lib/isc/work.c @@ -55,7 +55,8 @@ enum workstate { struct isc_work { unsigned int magic; - uint32_t state; /* enum workstate */ + uint32_t state; /* enum workstate */ + isc_result_t result; isc_work_cb cb; /* runs on a worker thread */ isc_work_done_cb done_cb; /* runs on the origin loop */ void *cbarg; @@ -191,12 +192,11 @@ static void work_done(void *arg) { isc_work_t *work = arg; isc_loop_t *loop = work->loop; - isc_result_t result = (uatomic_load(&work->state, CMM_ACQUIRE) != - WORK_CANCELED) - ? ISC_R_SUCCESS - : ISC_R_CANCELED; - work->done_cb(work->cbarg, result); + /* work_run() has settled work->result before scheduling us. */ + INSIST(work->result != ISC_R_UNSET); + + work->done_cb(work->cbarg, work->result); work->magic = 0; isc_mem_put(work->loop->mctx, work, sizeof(*work)); @@ -216,9 +216,10 @@ work_run(void *arg) { WORK_RUNNING); switch (prev) { case WORK_QUEUED: - work->cb(work->cbarg); + work->result = work->cb(work->cbarg); break; case WORK_CANCELED: + work->result = ISC_R_CANCELED; break; default: UNREACHABLE(); @@ -290,6 +291,7 @@ isc_work_enqueue(isc_loop_t *loop, isc_worklane_t lane, isc_work_cb cb, isc_work_t *work = isc_mem_get(loop->mctx, sizeof(*work)); *work = (isc_work_t){ .magic = WORK_MAGIC, + .result = ISC_R_UNSET, .cb = cb, .done_cb = done_cb, .cbarg = cbarg, @@ -331,7 +333,7 @@ isc_work_cancel(isc_work_t *work) { /* * Tombstone: QUEUED -> CANCELED. The node stays in the queue * (no interior unlink in a singly-linked lock-free queue) and - * is discarded by whichever worker dequeues it; after_cb still + * is discarded by whichever worker dequeues it; done_cb still * fires with ISC_R_CANCELED. Nothing is freed here. False * means the callback is running or done — uv_cancel semantics. */ diff --git a/tests/isc/work_test.c b/tests/isc/work_test.c index 9433ee6a6a2..cc6161e2ddc 100644 --- a/tests/isc/work_test.c +++ b/tests/isc/work_test.c @@ -39,15 +39,18 @@ static atomic_uint scheduled = 0; -static void +static isc_result_t work_cb(void *arg ISC_ATTR_UNUSED) { atomic_fetch_add(&scheduled, 1); assert_int_equal(isc_tid(), ISC_TID_UNKNOWN); + + return ISC_R_ALREADYRUNNING; /* mock result code */ } static void -after_work_cb(void *arg ISC_ATTR_UNUSED, isc_result_t result ISC_ATTR_UNUSED) { +after_work_cb(void *arg ISC_ATTR_UNUSED, isc_result_t result) { + assert_int_equal(result, ISC_R_ALREADYRUNNING); assert_int_equal(atomic_load(&scheduled), 1); isc_loopmgr_shutdown(); }