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@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(-)
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
