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