[libvirt] [PATCH 3/6] qemu: domain: Add helper for translating disk cachemode to qemu flags

[John Ferlan]

On 04/04/2018 04:13 AM, Peter Krempa wrote:
> Add helper which will map values of disk cache mode to the flags which
> are accepted by various parts of the qemu block layer.
> 
> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> ---
>  src/qemu/qemu_domain.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h |  6 ++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9d1c33b54a..20333c9568 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11920,6 +11920,81 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
>  }
> 
> 
> +/**
> + * qemuDomainDiskCachemodeFlags:
> + *
> + * Converts disk cachemode to the cache mode options for qemu. Returns -1 for
> + * invalid @cachemode values and fills the flags and returns 0 on success.
> + * Flags may be NULL.
> + */
> +int
> +qemuDomainDiskCachemodeFlags(int cachemode,
> +                             bool *writeback,
> +                             bool *direct,
> +                             bool *noflush)

Yuck, multiple boolean returns.... and when a new mode is added, we
possibly get another bool argument leading to another useless argument.
This could conceivably do nothing if each of the return args was NULL.

It seems what you're doing is translating or converting the
VIR_DOMAIN_DISK_CACHE_* values into some mask that you'll use elsewhere
perhaps similar to virDomainDefFormatConvertXMLFlags.

I think this just returns an unsigned int mask that's particular to some
to be defined enum that would set the right bits so that when building
the command line we can add features based on those bits.

That way it really doesn't matter that direct and noflush aren't used
(yet) in any patch that's been posted...

> +{
> +    bool dummy;
> +
> +    if (!writeback)
> +        writeback = &dummy;
> +
> +    if (!direct)
> +        direct = &dummy;
> +
> +    if (!noflush)
> +        noflush = &dummy;
> +
> +    /* Mapping of cache modes to the attributes according to qemu-options.hx
> +     *              │ cache.writeback   cache.direct   cache.no-flush
> +     * ─────────────┼─────────────────────────────────────────────────
> +     * writeback    │ true              false          false
> +     * none         │ true              true           false
> +     * writethrough │ false             false          false
> +     * directsync   │ false             true           false
> +     * unsafe       │ true              false          true
> +     */
> +    switch ((virDomainDiskCache) cachemode) {
> +    case VIR_DOMAIN_DISK_CACHE_DISABLE: /* 'none' */
> +        *writeback = true;
> +        *direct = true;
> +        *noflush = false;
> +        break;
> +
> +    case VIR_DOMAIN_DISK_CACHE_WRITETHRU:
> +        *writeback = false;
> +        *direct = false;
> +        *noflush = false;
> +        break;
> +
> +    case VIR_DOMAIN_DISK_CACHE_WRITEBACK:
> +        *writeback = true;
> +        *direct = false;
> +        *noflush = false;
> +        break;
> +
> +    case VIR_DOMAIN_DISK_CACHE_DIRECTSYNC:
> +        *writeback = false;
> +        *direct = true;
> +        *noflush = false;
> +        break;
> +
> +    case VIR_DOMAIN_DISK_CACHE_UNSAFE:
> +        *writeback = true;
> +        *direct = false;
> +        *noflush = true;
> +        break;
> +
> +    case VIR_DOMAIN_DISK_CACHE_DEFAULT:

Based on the patch 5 consumer qemuBuildDriveDevCacheStr - this would
DEFAULT would never happen.

> +    case VIR_DOMAIN_DISK_CACHE_LAST:
> +    default:

Considering how/when this helper is being used, one would hope by the
time this is used that cachemode has already been verified to be in
range. It would seem that's true because virDomainDiskDefDriverParseXML
would have failed the virDomainDiskCacheTypeFromString.

John

> +        virReportEnumRangeError(virDomainDiskCache, cachemode);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  void
>  qemuProcessEventFree(struct qemuProcessEvent *event)
>  {
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 21e12f6594..bbc2e83760 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -1003,4 +1003,10 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
>                              qemuDomainObjPrivatePtr priv,
>                              virQEMUDriverConfigPtr cfg);
> 
> +int
> +qemuDomainDiskCachemodeFlags(int cachemode,
> +                             bool *writeback,
> +                             bool *direct,
> +                             bool *noflush);
> +
>  #endif /* __QEMU_DOMAIN_H__ */
>