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

[Peter Krempa]

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 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.

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.
-------------- 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/20180417/06c33d79/attachment-0001.sig>