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

[Michal Privoznik]

On 8/19/20 2:00 PM, Erik Skultety wrote:
> On Wed, Aug 19, 2020 at 01:45:37PM +0200, Michal Privoznik wrote:
>> On 8/18/20 10:48 AM, Erik Skultety wrote:
>>> 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(-)
>>>>
>>
>>
>>> 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.
>>
>> Actually, I think maxQueuedJobs is job specific and not a global driver
>> setting. I mean, I can imagine other drivers benefiting from it too.
>> It's true that we currently don't allow specifying different values for
>> different domains and only apply the setting from qemu.conf, but in the
>> future we might make this available in domain XML somehow [1] and thus be
>> per domain. Moreover, I can imagine users wanting to set the limit for
>> volumes too [2].
> 
> I can relate to the reasoning, but I still don't think the Job structure is the
> right place, like I said, it holds everything that a single job needs, from
> that POV a single job doesn't really need to do anything with that value, it's
> just it happens to be a very convenient place right now, I say let's try
> finding a different location for the attribute where it makes more sense.

Sure, we can probably find different location, but question then is how 
usable the APIs would be? My idea of a good API is like this:

job = initJob(parameters);  /* alternatively, initJob(&job, parameters); */

beginJob(&job, type);

/* do something useful */

endJob(&job);

If we'd have maxQueuedJobs separate, then we would have to pass it in 
beginJob(), right?

Michal