Re: [libvirt] [PATCHv2] LXC: Fix handling of RAM filesystem size units

On 10/09/2013 07:22 AM, Daniel P. Berrange wrote: >> be used with <code>type='mount'</code>. The <code>usage</code> attribute >> - is used with <code>type='ram'</code> to set the memory limit in KB. >> + is used with <code>type='ram'</code> to set the memory limit KiB, unless >> + units are specified by the <code>unit</code> attribute. Why are you dropping 'in'? I agree that 'KiB' is better than 'KB', but still think you need 'in'. >> +++ b/docs/schemas/domaincommon.rng >> @@ -1736,6 +1736,11 @@ >> <ref name='unit'/> >> </attribute> >> </optional> >> + <optional> >> + <attribute name='units'> >> + <ref name='unit'/> >> + </attribute> >> + </optional> Only accept one or the other; don't allow validation if both are present: <optional> <choice> <attribute name='unit'>... <attribute name='units'>... </choice> </optional> >> @@ -5973,6 +5974,7 @@ virDomainFSDefParseXML(xmlNodePtr node, >> else if (def->type == VIR_DOMAIN_FS_TYPE_RAM) { >> usage = virXMLPropString(cur, "usage"); >> unit = virXMLPropString(cur, "unit"); >> + units = virXMLPropString(cur, "units"); > > We should just be dropping the 'unit' attribute - it was undocumented > and broken. I'm not sure I agree - for consistency, all the rest of our XML uses 'unit'. <memory unit='KiB'>1048576</memory> <currentMemory unit='KiB'>1048576</currentMemory> >> @@ -14764,7 +14770,7 @@ virDomainFSDefFormat(virBufferPtr buf, >> break; >> >> case VIR_DOMAIN_FS_TYPE_RAM: >> - virBufferAsprintf(buf, " <source usage='%lld' units='KiB'/>\n", >> + virBufferAsprintf(buf, " <source usage='%lld' unit='KiB'/>\n", >> def->usage / 1024); >> break; > > No, you should not be changing the attribute name here. Consistent output, as long as we accept exactly one of the two names on input, seems reasonable to me. Then again, if we released documentation of one particular spelling, it's better to preserve that spelling for back-compat, even if inconsistent. As we didn't document ANY name (whether 'unit' or 'units'), I'd much rather output a consistent name.