On Wed, Oct 09, 2013 at 07:36:16AM -0600, Eric Blake wrote:
> 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.
The current libvirt releases print 'units' and the libvirt-glib and
libvirt-sandbox libraries are parsing 'units', so renaming it to
'unit' will break that.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list