On Wed, 2021-08-11 at 15:11 +0200, Martin Kletzander wrote:
> On Wed, Aug 11, 2021 at 08:31:03PM +0800, Luke Yue wrote:
> > On Tue, 2021-08-10 at 17:49 +0200, Martin Kletzander wrote:
> > > On Thu, Jul 22, 2021 at 03:13:22PM +0800, Luke Yue wrote:
> > > > As in testDomainGetControlInfo, a background job should be
> > > > running
> > > > between 0-10000 seconds, so make the testDomainGetJobInfo
> > > > consistent
> > > > with it.
> > > >
> > > > Signed-off-by: Luke Yue <lukedyue@xxxxxxxxx>
> > > > ---
> > > > src/test/test_driver.c | 51
> > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 51 insertions(+)
> > > >
> > > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > > > index 892dc978f2..29b19d80f0 100644
> > > > --- a/src/test/test_driver.c
> > > > +++ b/src/test/test_driver.c
> > > > @@ -2604,6 +2604,56 @@ testDomainGetOSType(virDomainPtr dom
> > > > G_GNUC_UNUSED)
> > > > return ret;
> > > > }
> > > >
> > > > +static int
> > > > +testDomainGetJobInfoImpl(virDomainObj *dom,
> > > > + virDomainJobInfoPtr info)
> > > > +{
> > > > + testDomainObjPrivate *priv = dom->privateData;
> > > > +
> > > > + memset(info, 0, sizeof(*info));
> > > > +
> > > > + if (priv->seconds < 10000) {
> > > > + info->type = VIR_DOMAIN_JOB_BOUNDED;
> > > > + info->dataTotal = 10000;
> > > > + info->dataProcessed = priv->seconds;
> > > > + info->dataRemaining = 10000 - priv->seconds;
> > > > + info->timeElapsed = priv->seconds;
> > > > + info->timeRemaining = 10000 - priv->seconds;
> > > > + info->memTotal = 1048576;
> > > > + info->memProcessed = 1048576 - priv->seconds;
> > > > + info->memRemaining = priv->seconds;
> > > > + info->fileTotal = 2097152;
> > > > + info->fileProcessed = 2097152 - priv->seconds;
> > > > + info->fileRemaining = priv->seconds;
> > > > + } else if (priv->seconds < 30000 && priv->seconds >=
> > > > 10000) {
> > > > + info->type = VIR_DOMAIN_JOB_COMPLETED;
> > > > + } else {
> > > > + info->type = VIR_DOMAIN_JOB_NONE;
> > > > + }
> > > > +
> > >
> > > So this is an interesting approach. However because we are not
> > > doing
> > > different setups for different domains (all possible ones use the
> > > same
> > > PrivateAlloc function) we cannot test much here. But then when
> > > you
> > > think about changing the domain time for some domains it (even by
> > > just
> > > using virDomainSetTime API) the job output is very much non-
> > > intuitive.
> > > So I would rather prefer some extra struct member(s) to keep
> > > track of
> > > the job info. It can then be nicely used for testing
> > > domainGetJobInfo
> > > with flags as well as aborting and starting jobs in a more
> > > interested
> > > and I think useful way.
> > >
> > > Other than that this series looks fine.
> >
> > Thanks for the review! So if I understand you correctly, we have to
> > add
> > struct member(s) such as
> >
> > unsigned int jobType;
> >
> > to testDomainObjPrivate to store the job info rather than using
> > time.
>
> Yes, if you want you can even use more members if there's a use for
> them.
>
> > We can change it to VIR_DOMAIN_BLOCK_JOB_CANCELED when we use
> > virDomainAbortJob, and change it to other values when the APIs
> > should
> > start a job, such as virDomainBackupBegin (not implement yet
> > though).
> >
>
> Yes, and starting any jobs is not needed right now. But it is way
> clearer to see what the code does when testDomainAbortJob() does:
>
> priv->jobState = VIR_DOMAIN_JOB_CANCELLED
>
> compared to:
>
> priv->seconds = 10000
>
> and it is also easier to review because it is easy to gloss over the
> fact that priv->seconds == 10000 does not mean CANCELLED, but
> COMPLETED. Essentially more readable data structures make it easier
> to
> spot errors.
>
> > But I am a little bit puzzling about testing the domainGetJobInfo
> > with
> > flags, the virDomainGetJobInfo currently don't have flags as
> > parameter,
> > do we have to add one? Or do I misunderstand something?
> >
>
> Sorry, I meant JobStats, there you can get info for completed jobs
> and
> so on. Those are not that important for the test driver, but once
> the
> backend is implemented they should be trivial to adapt.
>
Thanks, I think I got your points now, I will try to implement them in
v2.
> > Thanks,
> > Luke
> >
> > >
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +testDomainGetJobInfo(virDomainPtr dom,
> > > > + virDomainJobInfoPtr info)
> > > > +{
> > > > + virDomainObj *vm;
> > > > + int ret = -1;
> > > > +
> > > > + if (!(vm = testDomObjFromDomain(dom)))
> > > > + goto cleanup;
> > > > +
> > > > + if (virDomainObjCheckActive(vm) < 0)
> > > > + goto cleanup;
> > > > +
> > > > + ret = testDomainGetJobInfoImpl(vm, info);
> > > > +
> > > > + cleanup:
> > > > + virDomainObjEndAPI(&vm);
> > > > + return ret;
> > > > +}
> > > > +
> > > >
> > > > static int
> > > > testDomainGetLaunchSecurityInfo(virDomainPtr domain
> > > > G_GNUC_UNUSED,
> > > > @@ -9484,6 +9534,7 @@ static virHypervisorDriver
> > > > testHypervisorDriver = {
> > > > .domainMemoryPeek = testDomainMemoryPeek, /* 5.4.0 */
> > > > .domainGetBlockInfo = testDomainGetBlockInfo, /* 5.7.0 */
> > > > .domainSetLifecycleAction = testDomainSetLifecycleAction,
> > > > /*
> > > > 5.7.0 */
> > > > + .domainGetJobInfo = testDomainGetJobInfo, /* 7.6.0 */
> > > >
> > > > .domainSnapshotNum = testDomainSnapshotNum, /* 1.1.4 */
> > > > .domainSnapshotListNames = testDomainSnapshotListNames, /*
> > > > 1.1.4 */
> > > > --
> > > > 2.32.0
> > > >
> >
