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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]