[libvirt] [PATCH 1/2] test_driver: implement virDomainGetBlockIoTune

[Erik Skultety]

On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis <stamatis.iliass at gmail.com>
> ---
>  src/test/test_driver.c | 90 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index ab0f8b06d6..9f4e255e35 100755
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -3500,6 +3500,95 @@ testDomainGetInterfaceParameters(virDomainPtr dom,
>      virDomainObjEndAPI(&vm);
>      return ret;
>  }
> +
> +
> +static int
> +testDomainGetBlockIoTune(virDomainPtr dom,
> +                         const char *path,
> +                         virTypedParameterPtr params,
> +                         int *nparams,
> +                         unsigned int flags)
> +{
> +    virDomainObjPtr vm = NULL;
> +    virDomainDefPtr def = NULL;
> +    virDomainDiskDefPtr disk;
> +    virDomainBlockIoTuneInfo reply = {0};
> +    int ret = -1;
> +
> +    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> +                  VIR_DOMAIN_AFFECT_CONFIG |
> +                  VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> +    flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
> +
> +    if (*nparams == 0) {
> +        *nparams = 20;
> +        return 0;
> +    }
> +
> +    if (!(vm = testDomObjFromDomain(dom)))
> +        return -1;
> +
> +    if (!(def = virDomainObjGetOneDef(vm, flags)))
> +        goto cleanup;
> +
> +    if (!(disk = virDomainDiskByName(def, path, true))) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("disk '%s' was not found in the domain config"),
> +                       path);
> +        goto cleanup;
> +    }
> +
> +    reply = disk->blkdeviotune;
> +    if (VIR_STRDUP(reply.group_name, disk->blkdeviotune.group_name) < 0)
> +        goto cleanup;
> +
> +#define ULL VIR_TYPED_PARAM_ULLONG

I don't see a point in doing ^this just to save a few characters, we're way
over the 80 columns already anyway, so wrap the long lines and span the macro
call over multiple lines.
Funny that I remember us having a discussion about macros doing string
concatenation (which I don't really mind that much) but prevents you from jumping
directly to the symbol definition - that's exactly what the QEMU alternative
does, I guess just to save some common prefixes :D.

Looking at the functions that use the typed parameters, an improvement we could
definitely do (in a standalone patch) is to ditch the index from the macro
definition since it's quite fragile, by doing:

int maxparams = X;

if (*nparams == 0) {
    *nparams = maxparams;
    return 0;
}

*nparams = 0;

TEST_SET_PARAM(&param[*nparams++],...)

> +    TEST_SET_PARAM(0, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, ULL, reply.total_bytes_sec);
> +    TEST_SET_PARAM(1, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, ULL, reply.read_bytes_sec);
> +    TEST_SET_PARAM(2, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, ULL, reply.write_bytes_sec);
> +
> +    TEST_SET_PARAM(3, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, ULL, reply.total_iops_sec);
> +    TEST_SET_PARAM(4, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, ULL, reply.read_iops_sec);
> +    TEST_SET_PARAM(5, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, ULL, reply.write_iops_sec);
> +
> +    TEST_SET_PARAM(6, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX, ULL, reply.total_bytes_sec_max);
> +    TEST_SET_PARAM(7, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX, ULL, reply.read_bytes_sec_max);
> +    TEST_SET_PARAM(8, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX, ULL, reply.write_bytes_sec_max);
> +
> +    TEST_SET_PARAM(9, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX, ULL, reply.total_iops_sec_max);
> +    TEST_SET_PARAM(10, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX, ULL, reply.read_iops_sec_max);
> +    TEST_SET_PARAM(11, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX, ULL, reply.write_iops_sec_max);
> +
> +    TEST_SET_PARAM(12, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, ULL, reply.size_iops_sec);
> +
> +    TEST_SET_PARAM(13, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, VIR_TYPED_PARAM_STRING, reply.group_name);
> +    reply.group_name = NULL;
> +
> +    TEST_SET_PARAM(14, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, ULL,
> +                   reply.total_bytes_sec_max_length);
> +    TEST_SET_PARAM(15, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, ULL,
> +                   reply.read_bytes_sec_max_length);
> +    TEST_SET_PARAM(16, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH, ULL,
> +                   reply.write_bytes_sec_max_length);
> +
> +    TEST_SET_PARAM(17, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH, ULL,
> +                   reply.total_iops_sec_max_length);
> +    TEST_SET_PARAM(18, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, ULL,
> +                   reply.read_iops_sec_max_length);
> +    TEST_SET_PARAM(19, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH, ULL,
> +                   reply.write_iops_sec_max_length);
> +#undef ULL
> +
> +    if (*nparams > 20)
> +        *nparams = 20;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(reply.group_name);
> +    virDomainObjEndAPI(&vm);
> +    return ret;
> +}
>  #undef TEST_SET_PARAM
>
>
> @@ -8512,6 +8601,7 @@ static virHypervisorDriver testHypervisorDriver = {
>      .domainGetNumaParameters = testDomainGetNumaParameters, /* 5.6.0 */
>      .domainSetInterfaceParameters = testDomainSetInterfaceParameters, /* 5.6.0 */
>      .domainGetInterfaceParameters = testDomainGetInterfaceParameters, /* 5.6.0 */
> +    .domainGetBlockIoTune = testDomainGetBlockIoTune, /* 5.6.0 */

5.7.0 (so that we/I don't forget about to change it ;))

With breaking the long lines:
Reviewed-by: Erik Skultety <eskultet at redhat.com>