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

[Deepak C Shetty]

On 01/12/2012 03:49 AM, Eric Blake wrote:
> 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).

Yes, thats correct, thats how qemu works today for writeout.
> 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>.
>

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

Agreed. will remove the <choice> pair
>> @@ -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).
>

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

I think leaving it there would make the code more understandable,
i prefer to leave it there.
>> @@ -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 ','.
>

Will do.
>> +/* 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 */
>

Will do.
>> +++ 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.
>

Will undo the unrelated changes.
>> -    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).
>
Will do.
>> @@ -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.
>

Will do.
>> +       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.
>

Thanks, will post v4 in some time.