Re: [libvirt] [PATCH v3] Add new attribute wrpolicy to <driver> element

I'm still trying to understand the qemu semantics: your choices are an explicit writeout=immediate (with explicit requests to the host) or nothing (host writes may be delayed). So, you are saying the a missing attribute lets qemu choose, and an explicit attribute maps to the explicit qemu values (where right now qemu only has one documented explicit value).

I guess that works, but I think this is a better wording for those semantics: [handle since] 0.9.7)</span>. The driver block has an optional attribute <code>wrpolicy</code> that further controls interaction with the host page cache; omitting the attribute gives default behavior, while the value<code>immediate</code> means that a host writeback is immediately triggered for all pages touched during a guest file write operation <span class="since">(since 0.9.10)</span>. Then, supposing we add a new policy 'foo' in the future, we could change things to say: The driver block has an optional attribute <code>wrpolicy</code> that further controls interaction with the host page cache; omitting the attribute gives default behavior, the value <code>immediate</code> means that a host writeback is immediately triggered for all pages touched during a guest file write operation, and the value<code>foo</code> does xxyzy<span class="since">(since 0.9.10, wrpolicy='foo' since 1.0)</span>.

> +++ b/docs/schemas/domaincommon.rng > @@ -1142,6 +1142,13 @@ > <value>handle</value> > </choice> > </attribute> > +<optional> > +<attribute name="wrpolicy"> > +<choice> > +<value>immediate</value> > +</choice> No need for a<choice> when there's only one possible value; then again, leaving the<choice> in now means we won't have to reindent if a new value is added later. Up to you if you want to shave two lines of the patch.

> @@ -3530,6 +3535,16 @@ virDomainFSDefParseXML(xmlNodePtr node, > } > } > > + if (wrpolicy) { > + if ((def->wrpolicy = virDomainFSWrpolicyTypeFromString(wrpolicy))< 0) { This must be<= 0, not< 0 (that is, your RNG didn't allow wrpolicy='default', so neither should the code).

> + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown filesystem write policy '%s'"), wrpolicy); > + goto error; > + } > + } else { > + def->wrpolicy = VIR_DOMAIN_FS_WRPOLICY_DEFAULT; This else clause is redundant: VIR_DOMAIN_FS_WRPOLICY_DEFAULT is 0, but def was 0-initialized. But I guess it doesn't hurt to leave it in.

> @@ -10182,7 +10199,15 @@ virDomainFSDefFormat(virBufferPtr buf, > type, accessmode); > > if (def->fsdriver) { > - virBufferAsprintf(buf, "<driver type='%s'/>\n", fsdriver); > + virBufferAsprintf(buf, "<driver type='%s'", fsdriver); > + > + /* Don't generate anything if wrpolicy is set to default */ > + if (def->wrpolicy) { > + virBufferAsprintf(buf, " wrpolicy='%s'", wrpolicy); > + } > + > + virBufferAddLit(buf,"/>\n"); Style - space after ','.

> +/* Filesystem Write policy */ > +enum virDomainFSWrpolicy { > + VIR_DOMAIN_FS_WRPOLICY_DEFAULT = 0, > + VIR_DOMAIN_FS_WRPOLICY_IMMEDIATE, > + > + VIR_DOMAIN_FS_WRPOLICY_LAST > +}; > + > typedef struct _virDomainFSDef virDomainFSDef; > typedef virDomainFSDef *virDomainFSDefPtr; > struct _virDomainFSDef { > int type; > int fsdriver; > int accessmode; > + int wrpolicy; Lately, I've been annotating what values _should_ go in an int field member, as in: int wrpolicy; /* enum virDomainFSWrpolicy */

> +++ b/src/qemu/qemu_capabilities.h > @@ -113,10 +113,11 @@ enum qemuCapsFlags { > QEMU_CAPS_NO_SHUTDOWN = 74, /* usable -no-shutdown */ > > QEMU_CAPS_DRIVE_CACHE_UNSAFE = 75, /* Is cache=unsafe supported? */ > - QEMU_CAPS_PCI_ROMBAR = 76, /* -device rombar=0|1 */ > + QEMU_CAPS_PCI_ROMBAR = 76, /* -device rombar=0|1 */ > QEMU_CAPS_ICH9_AHCI = 77, /* -device ich9-ahci */ > QEMU_CAPS_NO_ACPI = 78, /* -no-acpi */ Why did you change spacing on QEMU_CAPS_PCI_ROMBAR, but not on QEMU_CAPS_NO_ACPI? If you're going to make an unrelated change, at least make all the code in that neighborhood consistent. It's almost worth doing the whitespace changes in a separate patch, where we can make the entire enum consistent.

> - QEMU_CAPS_FSDEV_READONLY =79, /* -fsdev readonly supported */ > + QEMU_CAPS_FSDEV_READONLY = 79, /* -fsdev readonly supported */ > + QEMU_CAPS_FSDEV_WRITEOUT = 80, /* -fsdev writeout supported */ > > QEMU_CAPS_LAST, /* this must always be the last item */ > }; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 73f673f..ba0353e 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -108,6 +108,10 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, > "local", > "handle"); > > +VIR_ENUM_DECL(qemuDomainFSWrpolicy) > +VIR_ENUM_IMPL(qemuDomainFSWrpolicy, VIR_DOMAIN_FS_WRPOLICY_LAST, > + "default", > + "immediate"); No need for this; rather, make sure libvirt_private.syms exports virDomainFSWrpolicyTypeToString, and reuse that function. (A qemu-specific version is only needed if the mapping of the XML names to the qemu attributes differs, but right now, our mapping of the single element "immediate" is the same).

> @@ -2119,6 +2124,15 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, > } > } > > + if (fs->wrpolicy != VIR_DOMAIN_FS_WRPOLICY_DEFAULT) { This can be simplified to: if (fs->wrpolicy) { since we already have the convention that VIR_DOMAIN_FS_WRPOLICY_DEFAULT was explicitly chosen as 0.

> + if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV_WRITEOUT)) { > + virBufferAsprintf(&opt, ",writeout=%s", wrpolicy); > + } else { > + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("filesystem writeout not supported")); > + } Missing a goto error. I think your choice of<driver wrpolicy='immediate'/> is reasonable; you may want to wait just a bit longer to see if any other opinions on naming choices are presented, or you can go ahead and post a v4 that incorporates fixes for my findings. At any rate, I think we'll get some form of this patch in for 0.9.10.