Re: [PATCH v6 29/33] job.h: assertions in the callers of JobDriver funcion pointers
On 28.01.22 16:19, Emanuele Giuseppe Esposito wrote: On 26/01/2022 15:13, Hanna Reitz wrote: On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote: Signed-off-by: Emanuele Giuseppe Esposito --- job.c | 9 + 1 file changed, 9 insertions(+) Just curious, why did you remove the assertion in job_co_entry()? (Looking at it again, it might have been nicer to swap it with the assertion below it, so that `job != NULL` is asserted first, but other than that...) I think it's useless, job_co_entry runs in a coroutine in job->aio_context created and entered in job_start (its only caller), so there is no way that we are in a different aiocontext. Well, that is what assertions are for. You don’t put them there to catch errors, you put them there to show that there’s some sort of contract the caller definitely fulfills, so that after the assertion you know this contract is fulfilled without having to read and verify what the caller does. And perhaps also to prevent regressions when the caller code changes. Same as assert(job), I don't think the opaque pointer can ever be NULL. Good, because if it could, that shouldn’t be an assertion. O:) Thank you, Emanuele (And since I’m already replying to this patch, might as well point out s/funcion/function/ in the subject) Hanna
Re: [PATCH v6 29/33] job.h: assertions in the callers of JobDriver funcion pointers
On 26/01/2022 15:13, Hanna Reitz wrote: > On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote: >> Signed-off-by: Emanuele Giuseppe Esposito >> --- >> job.c | 9 + >> 1 file changed, 9 insertions(+) > > Just curious, why did you remove the assertion in job_co_entry()? > (Looking at it again, it might have been nicer to swap it with the > assertion below it, so that `job != NULL` is asserted first, but other > than that...) > I think it's useless, job_co_entry runs in a coroutine in job->aio_context created and entered in job_start (its only caller), so there is no way that we are in a different aiocontext. Same as assert(job), I don't think the opaque pointer can ever be NULL. Thank you, Emanuele > (And since I’m already replying to this patch, might as well point out > s/funcion/function/ in the subject) > > Hanna >
Re: [PATCH v6 29/33] job.h: assertions in the callers of JobDriver funcion pointers
On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote: Signed-off-by: Emanuele Giuseppe Esposito --- job.c | 9 + 1 file changed, 9 insertions(+) Just curious, why did you remove the assertion in job_co_entry()? (Looking at it again, it might have been nicer to swap it with the assertion below it, so that `job != NULL` is asserted first, but other than that...) (And since I’m already replying to this patch, might as well point out s/funcion/function/ in the subject) Hanna
[PATCH v6 29/33] job.h: assertions in the callers of JobDriver funcion pointers
Signed-off-by: Emanuele Giuseppe Esposito
---
job.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/job.c b/job.c
index 54db80df66..39bf511949 100644
--- a/job.c
+++ b/job.c
@@ -381,6 +381,8 @@ void job_ref(Job *job)
void job_unref(Job *job)
{
+assert(qemu_in_main_thread());
+
if (--job->refcnt == 0) {
assert(job->status == JOB_STATUS_NULL);
assert(!timer_pending(&job->sleep_timer));
@@ -602,6 +604,7 @@ bool job_user_paused(Job *job)
void job_user_resume(Job *job, Error **errp)
{
assert(job);
+assert(qemu_in_main_thread());
if (!job->user_paused || job->pause_count <= 0) {
error_setg(errp, "Can't resume a job that was not paused");
return;
@@ -672,6 +675,7 @@ static void job_update_rc(Job *job)
static void job_commit(Job *job)
{
assert(!job->ret);
+assert(qemu_in_main_thread());
if (job->driver->commit) {
job->driver->commit(job);
}
@@ -680,6 +684,7 @@ static void job_commit(Job *job)
static void job_abort(Job *job)
{
assert(job->ret);
+assert(qemu_in_main_thread());
if (job->driver->abort) {
job->driver->abort(job);
}
@@ -687,6 +692,7 @@ static void job_abort(Job *job)
static void job_clean(Job *job)
{
+assert(qemu_in_main_thread());
if (job->driver->clean) {
job->driver->clean(job);
}
@@ -726,6 +732,7 @@ static int job_finalize_single(Job *job)
static void job_cancel_async(Job *job, bool force)
{
+assert(qemu_in_main_thread());
if (job->driver->cancel) {
force = job->driver->cancel(job, force);
} else {
@@ -825,6 +832,7 @@ static void job_completed_txn_abort(Job *job)
static int job_prepare(Job *job)
{
+assert(qemu_in_main_thread());
if (job->ret == 0 && job->driver->prepare) {
job->ret = job->driver->prepare(job);
job_update_rc(job);
@@ -1054,6 +1062,7 @@ void job_complete(Job *job, Error **errp)
{
/* Should not be reachable via external interface for internal jobs */
assert(job->id);
+assert(qemu_in_main_thread());
if (job_apply_verb(job, JOB_VERB_COMPLETE, errp)) {
return;
}
--
2.31.1
