[GSoC][PATCH v2 3/6] qemu_domainjob: `maxQueuedJobs` added to `qemuDomainJobPrivate`

[Erik Skultety]

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 at gmail.com>
> ---
>  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