[PATCH 1/4] test_driver: Implement virDomainGetJobInfo

[Martin Kletzander]

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,
>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
>> >
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210811/67ff6288/attachment-0001.sig>