On Mon, Aug 17, 2020 at 10:37:18AM +0530, Prathamesh Chavan wrote:
> Reference to `maxQueuedJobs` required us to access
> config of the qemu-driver. And creating its copy in
> the `qemuDomainJob` helped us access the variable
> without referencing the driver's config.
>
> Signed-off-by: Prathamesh Chavan <pc44800@xxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 5 ++++-
> src/qemu/qemu_domainjob.c | 13 +++++++------
> src/qemu/qemu_domainjob.h | 4 +++-
> 3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 1ae44ae39f..677fa7ea91 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2085,11 +2085,14 @@ static void *
> qemuDomainObjPrivateAlloc(void *opaque)
> {
> qemuDomainObjPrivatePtr priv;
> + virQEMUDriverPtr driver = opaque;
> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>
> if (VIR_ALLOC(priv) < 0)
> return NULL;
>
> - if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks) < 0) {
> + if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks,
> + cfg->maxQueuedJobs) < 0) {
> virReportSystemError(errno, "%s",
> _("Unable to init qemu driver mutexes"));
> goto error;
> diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
> index 7cd1aabd9e..eebc144747 100644
> --- a/src/qemu/qemu_domainjob.c
> +++ b/src/qemu/qemu_domainjob.c
> @@ -117,10 +117,12 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
>
> int
> qemuDomainObjInitJob(qemuDomainJobObjPtr job,
> - qemuDomainObjPrivateJobCallbacksPtr cb)
> + qemuDomainObjPrivateJobCallbacksPtr cb,
> + unsigned int maxQueuedJobs)
> {
> memset(job, 0, sizeof(*job));
> job->cb = cb;
> + job->maxQueuedJobs = maxQueuedJobs;
>
> if (!(job->privateData = job->cb->allocJobPrivate()))
> return -1;
> @@ -344,7 +346,6 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
> unsigned long long then;
> bool nested = job == QEMU_JOB_ASYNC_NESTED;
> bool async = job == QEMU_JOB_ASYNC;
> - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> const char *blocker = NULL;
> const char *agentBlocker = NULL;
> int ret = -1;
> @@ -370,8 +371,8 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>
> retry:
> if ((!async && job != QEMU_JOB_DESTROY) &&
> - cfg->maxQueuedJobs &&
> - priv->job.jobs_queued > cfg->maxQueuedJobs) {
> + priv->job.maxQueuedJobs &&
> + priv->job.jobs_queued > priv->job.maxQueuedJobs) {
> goto error;
> }
>
> @@ -501,8 +502,8 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
> _("cannot acquire state change lock"));
> }
> ret = -2;
> - } else if (cfg->maxQueuedJobs &&
> - priv->job.jobs_queued > cfg->maxQueuedJobs) {
> + } else if (priv->job.maxQueuedJobs &&
> + priv->job.jobs_queued > priv->job.maxQueuedJobs) {
> if (blocker && agentBlocker) {
> virReportError(VIR_ERR_OPERATION_FAILED,
> _("cannot acquire state change "
> diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h
> index 0696b79fe3..11e7f2f432 100644
> --- a/src/qemu/qemu_domainjob.h
> +++ b/src/qemu/qemu_domainjob.h
> @@ -153,6 +153,7 @@ struct _qemuDomainJobObj {
> unsigned long apiFlags; /* flags passed to the API which started the async job */
>
> int jobs_queued;
> + unsigned int maxQueuedJobs;
>
> void *privateData; /* job specific collection of data */
> qemuDomainObjPrivateJobCallbacksPtr cb;
The comment below applies to both 2/6 and 3/6.
Looking at this again, I don't think we want to move any of the jobs_queued or
maxQueuedJobs attributes to the job structure, I'm sorry I didn't spot this
right away in the previous revision of the series, I take responsibility for
it.
The job structure represents a single job, so from the design POV,
referencing both jobs_queued (which is really tied to a domain) and
maxQueuedJobs (which is a global driver setting) from within the job structure
simply doesn't feel right. Instead, I think qemuDomainObjBeginJobInternal will
simply have to be split in parts specific to QEMU that have access to the
driver and hypervisor-agnostic bits that can be moved to src/hypervisor.
Erik
