7:43 a.m.
On Wed, Apr 11, 2018 at 11:17:46 -0400, John Ferlan wrote:
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(a)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.
Exactly. Even the function comment says that.
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.
It can't return unsigned since due to the new switch/case handling for
enums we need to be able return failure.
Also re-compressing this to a bitmap is obviously possible, but rather
pointless, since any user will need to re-extract the presence of
individual bits.
That way it really doesn't matter that direct and noflush
aren't used
(yet) in any patch that's been posted...
I fail to see how that's different from re-extracting them from a
bitmap.
> +{
> + 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.
No, callers only verify one end of the range. The other end of the range
is not verified. Also we recently decided to always include the default
case even when we validate the callers or fully enumerate all values,
since it does not prevent assigning any invalid value.