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

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(a)gmail.com&gt; >>> --- >>> 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.