[PATCH 1/4] test_driver: Implement virDomainGetJobInfo

[Luke Yue]

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 at gmail.com>
> > > > ---
> > > > 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
> > > > 
> >