Re: [PATCH 1/4] test_driver: Implement virDomainGetJobInfo
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(a)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.
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
>