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.
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).
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?
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
> >
