Keeping the above arguments in mind, I think having callback functions for accessing them instead of added them should do the work for now. Thanks, Prathamesh Chavan On Wed, Aug 19, 2020 at 9:05 PM Erik Skultety <eskultet@xxxxxxxxxx> wrote: > > On Wed, Aug 19, 2020 at 05:04:58PM +0200, Michal Privoznik wrote: > > 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? > > Naturally. In a standard OOP design, such information would come from the > object calling the beginJob() method, in our case this would most likely > translate to the domain object over which we're executing a job. We'd therefore > need the domain object as well. > > I see what you mean, but a usable API is as important as logically structured > data the API takes. > > Erik >
