Re: [GSoC][PATCH v4 3/4] qemu_domainjob: introduce `privateData` for `qemuDomainJob`

On 7/13/20 8:03 PM, Prathamesh Chavan wrote: Yeah, well callbacks should be static thus should live in the qemu_domain.c too. Also, the private data struct is defined in qemu_domain.h and thus callbacks which allocate/free/format/parse should live in the corresponding .c file. This looks almost perfect. I think we will need one callback more - resetJobPrivate. More below. Also, there is no need to put ampersand before functions. Compilers are wise enough to tell that we need the address of the functions. Or plain: g_new0(qemuDomainJobPrivate, 1) See, this is what previously qemuDomainObjResetAsyncJob() was doing. And while qemuDomainObjResetAsyncJob() is called from qemuDomainObjFreeJob() it is also called from qemuDomainObjBeginJobInternal() and other places. Therefore, naming this Free() is probably not the best idea. It should be named qemuJobResetPrivate() or something, esp. beacuse it doesn't really free the private data (it doesn't call free(priv)), it just clears/resets them. And this will actually free the data. This needs to call the resetJobPrivate() callback then. Ooops, we definitely do not want to this. Because if we did, then the second time this is called we would dereference a NULL pointer when trying to call any of the callbacks (resetJobPrivate() for instance). Okay, I have to admit, this took me a while to figure out. And it is sort of corect. BUT, after this the priv->job.privateData is going to be NULL (because that's how g_steal_pointer() works). And I don't think the code is prepared for that. So what we need to do is to allocate new private data here. This call is needless - alloc failed there is nothing to free. But if any of these calls fail, then we have allocated data the needs freeing. I think we want this to be squashed in: diff --git c/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c index a02865dc59..b0ed562d25 100644 --- c/src/qemu/qemu_domain.c +++ w/src/qemu/qemu_domain.c @@ -88,11 +88,72 @@ VIR_ENUM_IMPL(qemuDomainNamespace, "mount", ); + +static void * +qemuJobAllocPrivate(void) +{ + return g_new0(qemuDomainJobPrivate, 1); +} + + +static void +qemuJobFreePrivate(void *opaque) +{ + qemuDomainJobPrivatePtr priv = opaque; + + if (!priv) + return; + + qemuMigrationParamsFree(priv->migParams); + VIR_FREE(priv); +} + + +static void +qemuJobResetPrivate(void *opaque) +{ + qemuDomainJobPrivatePtr priv = opaque; + + priv->spiceMigration = false; + priv->spiceMigrated = false; + priv->dumpCompleted = false; + qemuMigrationParamsFree(priv->migParams); + priv->migParams = NULL; +} + + +static int +qemuDomainFormatJobPrivate(virBufferPtr buf, + qemuDomainJobObjPtr job) +{ + qemuDomainJobPrivatePtr priv = job->privateData; + + if (priv->migParams) + qemuMigrationParamsFormat(buf, priv->migParams); + + return 0; +} + + +static int +qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt, + qemuDomainJobObjPtr job) +{ + qemuDomainJobPrivatePtr priv = job->privateData; + + if (qemuMigrationParamsParse(ctxt, &priv->migParams) < 0) + return -1; + + return 0; +} + + static qemuDomainObjPrivateJobCallbacks qemuPrivateJobCallbacks = { - .allocJobPrivate = &qemuJobAllocPrivate, - .freeJobPrivate = &qemuJobFreePrivate, - .formatJob = &qemuDomainFormatJobPrivate, - .parseJob = &qemuDomainParseJobPrivate, + .allocJobPrivate = qemuJobAllocPrivate, + .freeJobPrivate = qemuJobFreePrivate, + .resetJobPrivate = qemuJobResetPrivate, + .formatJob = qemuDomainFormatJobPrivate, + .parseJob = qemuDomainParseJobPrivate, }; /** diff --git c/src/qemu/qemu_domainjob.c w/src/qemu/qemu_domainjob.c index fe160afe79..64381d8f78 100644 --- c/src/qemu/qemu_domainjob.c +++ w/src/qemu/qemu_domainjob.c @@ -159,33 +159,6 @@ qemuDomainEventEmitJobCompleted(virQEMUDriverPtr driver, virObjectEventStateQueue(driver->domainEventState, event); } -void * -qemuJobAllocPrivate(void) -{ - qemuDomainJobPrivatePtr priv; - if (VIR_ALLOC(priv) < 0) - return NULL; - return (void *)priv; -} - - -static void -qemuJobFreePrivateData(qemuDomainJobPrivatePtr priv) -{ - priv->spiceMigration = false; - priv->spiceMigrated = false; - priv->dumpCompleted = false; - qemuMigrationParamsFree(priv->migParams); - priv->migParams = NULL; -} - -void -qemuJobFreePrivate(void *opaque) -{ - qemuDomainJobObjPtr job = (qemuDomainJobObjPtr) opaque; - qemuJobFreePrivateData(job->privateData); -} - static void qemuDomainObjResetJob(qemuDomainJobObjPtr job) { @@ -218,12 +191,12 @@ qemuDomainObjResetAsyncJob(qemuDomainJobObjPtr job) job->abortJob = false; VIR_FREE(job->error); g_clear_pointer(&job->current, qemuDomainJobInfoFree); - job->cb->freeJobPrivate(job); + job->cb->resetJobPrivate(job->privateData); job->apiFlags = 0; - job->cb = NULL; } -void + +int qemuDomainObjRestoreJob(virDomainObjPtr obj, qemuDomainJobObjPtr job) { @@ -238,8 +211,13 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj, job->privateData = g_steal_pointer(&priv->job.privateData); job->apiFlags = priv->job.apiFlags; + if (!(priv->job.privateData = priv->job.cb->allocJobPrivate())) + return -1; + job->cb = priv->job.cb; + qemuDomainObjResetJob(&priv->job); qemuDomainObjResetAsyncJob(&priv->job); + return 0; } void @@ -247,6 +225,7 @@ qemuDomainObjFreeJob(qemuDomainJobObjPtr job) { qemuDomainObjResetJob(job); qemuDomainObjResetAsyncJob(job); + job->cb->freeJobPrivate(job->privateData); g_clear_pointer(&job->current, qemuDomainJobInfoFree); g_clear_pointer(&job->completed, qemuDomainJobInfoFree); virCondDestroy(&job->cond); @@ -1245,15 +1224,6 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf, return 0; } -int -qemuDomainFormatJobPrivate(virBufferPtr buf, - qemuDomainJobObjPtr job) -{ - qemuDomainJobPrivatePtr priv = job->privateData; - if (priv->migParams) - qemuMigrationParamsFormat(buf, priv->migParams); - return 0; -} int qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, @@ -1382,16 +1352,6 @@ qemuDomainObjPrivateXMLParseJobNBD(virDomainObjPtr vm, return 0; } -int -qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt, - qemuDomainJobObjPtr job) -{ - qemuDomainJobPrivatePtr jobPriv = job->privateData; - if (qemuMigrationParamsParse(ctxt, &jobPriv->migParams) < 0) - return -1; - return 0; -} - int qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm, xmlXPathContextPtr ctxt) @@ -1460,15 +1420,16 @@ qemuDomainObjInitJob(qemuDomainJobObjPtr job, memset(job, 0, sizeof(*job)); job->cb = cb; - if (!(job->privateData = job->cb->allocJobPrivate())) { - qemuDomainObjFreeJob(job); + if (!(job->privateData = job->cb->allocJobPrivate())) return -1; - } - if (virCondInit(&job->cond) < 0) + if (virCondInit(&job->cond) < 0) { + job->cb->freeJobPrivate(job->privateData); return -1; + } if (virCondInit(&job->asyncCond) < 0) { + job->cb->freeJobPrivate(job->privateData); virCondDestroy(&job->cond); return -1; } diff --git c/src/qemu/qemu_domainjob.h w/src/qemu/qemu_domainjob.h index b0e840adc8..d2c599cd68 100644 --- c/src/qemu/qemu_domainjob.h +++ w/src/qemu/qemu_domainjob.h @@ -159,6 +159,7 @@ typedef qemuDomainJobObj *qemuDomainJobObjPtr; typedef void *(*qemuDomainObjPrivateJobAlloc)(void); typedef void (*qemuDomainObjPrivateJobFree)(void *); +typedef void (*qemuDomainObjPrivateJobReset)(void *); typedef int (*qemuDomainObjPrivateJobFormat)(virBufferPtr, qemuDomainJobObjPtr); typedef int (*qemuDomainObjPrivateJobParse)(xmlXPathContextPtr, @@ -169,6 +170,7 @@ typedef qemuDomainObjPrivateJobCallbacks *qemuDomainObjPrivateJobCallbacksPtr; struct _qemuDomainObjPrivateJobCallbacks { qemuDomainObjPrivateJobAlloc allocJobPrivate; qemuDomainObjPrivateJobFree freeJobPrivate; + qemuDomainObjPrivateJobReset resetJobPrivate; qemuDomainObjPrivateJobFormat formatJob; qemuDomainObjPrivateJobParse parseJob; }; @@ -248,8 +250,8 @@ void qemuDomainObjSetJobPhase(virQEMUDriverPtr driver, int phase); void qemuDomainObjSetAsyncJobMask(virDomainObjPtr obj, unsigned long long allowedJobs); -void qemuDomainObjRestoreJob(virDomainObjPtr obj, - qemuDomainJobObjPtr job); +int qemuDomainObjRestoreJob(virDomainObjPtr obj, + qemuDomainJobObjPtr job); void qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj); void qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj); @@ -291,17 +293,3 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, int qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm, xmlXPathContextPtr ctxt); - -void * -qemuJobAllocPrivate(void); - -void -qemuJobFreePrivate(void *opaque); - -int -qemuDomainFormatJobPrivate(virBufferPtr buf, - qemuDomainJobObjPtr job); - -int -qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt, - qemuDomainJobObjPtr job); Michal