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

Re: [libvirt] [PATCH go-xml] add support for domain features



On Thu, May 04, 2017 at 12:43:47PM -0700, Ryan Goodfellow wrote:
> ping, ref <
> https://www.redhat.com/archives/libvir-list/2017-April/msg01073.html>

Oh, sorry, I didn't realize that was a follow up to this - usually we would
recommand posting a complete standalone v2 patch, rather than just posting
incremental changes. 

> 
> On Thu, Apr 27, 2017 at 12:35 PM, Ryan Goodfellow <rgoodfel isi edu> wrote:
> 
> > Hi Daniel,
> >
> > I have addressed the issues you brought up in the commit titled 'remove
> > superfluous state & omitempty entries' that has been posted to the list.
> >
> > --
> > cheers
> > ry
> >
> > On Mon, Apr 24, 2017 at 3:47 AM, Daniel P. Berrange <berrange redhat com>
> > wrote:
> >
> >> On Sat, Apr 22, 2017 at 02:53:48PM -0700, Ryan Goodfellow wrote:
> >> > This commit adds support for domain features. It does so by introducing
> >> > a new family of types DomainFeature*. The aggregate type
> >> > DomainFeatureList has been added to the Domain type to plumb in the new
> >> > type family. Testing has also been added in domain_test.go
> >> > ---
> >> >  domain.go      | 91 ++++++++++++++++++++++++++++++
> >> +++++++++++++++++-----------
> >> >  domain_test.go | 55 +++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 130 insertions(+), 16 deletions(-)
> >> >
> >> > diff --git a/domain.go b/domain.go
> >> > index cccd9a6..c9ffaef 100644
> >> > --- a/domain.go
> >> > +++ b/domain.go
> >> > @@ -371,23 +371,82 @@ type DomainCPU struct {
> >> >       Features []DomainCPUFeature `xml:"feature"`
> >> >  }
> >> >
> >> > +type DomainFeature struct {
> >> > +     State string `xml:"state,attr,omitempty"`
> >> > +}
> >>
> >> There is no 'state' attribute on most top level features - this is just
> >> something used under the hyperv features....
> >>
> >> > +
> >> > +type DomainFeatureAPIC struct {
> >> > +     DomainFeature
> >> > +     EOI string `xml:"eio,attr,omitempty"`
> >> > +}
> >>
> >> So this is wrong for example
> >>
> >> > +
> >> > +type DomainFeatureVendorId struct {
> >> > +     DomainFeature
> >> > +     Value string `xml:"value,attr,omitempty"`
> >> > +}
> >> > +
> >> > +type DomainFeatureSpinlocks struct {
> >> > +     DomainFeature
> >> > +     Retries uint `xml:"retries,attr,omitempty"`
> >> > +}
> >>
> >> Since these are hyperv featurs, the struct name should have
> >> HyperV in the name too.
> >>
> >> > +
> >> > +type DomainFeatureHyperV struct {
> >> > +     DomainFeature
> >> > +     Relaxed   *DomainFeature          `xml:"relaxed,omitempty"`
> >> > +     VAPIC     *DomainFeature          `xml:"vapic,omitempty"`
> >> > +     Spinlocks *DomainFeatureSpinlocks `xml:"spinlocks,omitempty"`
> >> > +     VPIndex   *DomainFeature          `xml:"vpindex,omitempty"`
> >> > +     Runtime   *DomainFeature          `xml:"runtime,omitempty"`
> >> > +     Synic     *DomainFeature          `xml:"synic,omitempty"`
> >> > +     STimer    *DomainFeature          `xml:"stimer,omitempty"`
> >> > +     Reset     *DomainFeature          `xml:"reset,omitempty"`
> >> > +     VendorId  *DomainFeatureVendorId  `xml:"vendor_id,omitempty"`
> >>
> >> There's no need to use 'omitempty' when the type is a struct
> >> pointer - its only needed for scalar types.
> >>
> >> > +}
> >> > +
> >> > +type DomainFeatureKVM struct {
> >> > +     DomainFeature
> >> > +     Hidden *DomainFeature `xml:"hidden,omitempty"`
> >> > +}
> >> > +
> >> > +type DomainFeatureGIC struct {
> >> > +     DomainFeature
> >> > +     Version string `xml:"version,attr,omitempty"`
> >> > +}
> >> > +
> >> > +type DomainFeatureList struct {
> >> > +     PAE        *DomainFeature       `xml:"pae,omitempty"`
> >> > +     ACPI       *DomainFeature       `xml:"acpi,omitempty"`
> >> > +     APIC       *DomainFeatureAPIC   `xml:"apic,omitempty"`
> >> > +     HAP        *DomainFeature       `xml:"hap,omitempty"`
> >> > +     Viridian   *DomainFeature       `xml:"viridian,omitempty"`
> >> > +     PrivNet    *DomainFeature       `xml:"privnet,omitempty"`
> >> > +     HyperV     *DomainFeatureHyperV `xml:"hyperv,omitempty"`
> >> > +     KVM        *DomainFeatureKVM    `xml:"kvm,omitempty"`
> >> > +     PVSpinlock *DomainFeature       `xml:"pvspinlock,omitempty"`
> >> > +     PMU        *DomainFeature       `xml:"pmu,omitempty"`
> >> > +     VMPort     *DomainFeature       `xml:"vmport,omitempty"`
> >> > +     GIC        *DomainFeatureGIC    `xml:"gic,omitempty"`
> >> > +     SMM        *DomainFeature       `xml:"smm,omitempty"`
> >> > +}
> >>
> >> None of these should use 'DomainFeature', since they do not
> >> want a 'state' attribute. Also same note about omitempty not
> >> being needed.
> >>
> >> > +
> >> >  type Domain struct {
> >> > -     XMLName       xml.Name          `xml:"domain"`
> >> > -     Type          string            `xml:"type,attr,omitempty"`
> >> > -     Name          string            `xml:"name"`
> >> > -     UUID          string            `xml:"uuid,omitempty"`
> >> > -     Memory        *DomainMemory     `xml:"memory"`
> >> > -     CurrentMemory *DomainMemory     `xml:"currentMemory"`
> >> > -     MaximumMemory *DomainMaxMemory  `xml:"maxMemory"`
> >> > -     VCPU          *DomainVCPU       `xml:"vcpu"`
> >> > -     CPU           *DomainCPU        `xml:"cpu"`
> >> > -     Resource      *DomainResource   `xml:"resource"`
> >> > -     Devices       *DomainDeviceList `xml:"devices"`
> >> > -     OS            *DomainOS         `xml:"os"`
> >> > -     SysInfo       *DomainSysInfo    `xml:"sysinfo"`
> >> > -     OnPoweroff    string            `xml:"on_poweroff,omitempty"`
> >> > -     OnReboot      string            `xml:"on_reboot,omitempty"`
> >> > -     OnCrash       string            `xml:"on_crash,omitempty"`
> >> > +     XMLName       xml.Name           `xml:"domain"`
> >> > +     Type          string             `xml:"type,attr,omitempty"`
> >> > +     Name          string             `xml:"name"`
> >> > +     UUID          string             `xml:"uuid,omitempty"`
> >> > +     Memory        *DomainMemory      `xml:"memory"`
> >> > +     CurrentMemory *DomainMemory      `xml:"currentMemory"`
> >> > +     MaximumMemory *DomainMaxMemory   `xml:"maxMemory"`
> >> > +     VCPU          *DomainVCPU        `xml:"vcpu"`
> >> > +     CPU           *DomainCPU         `xml:"cpu"`
> >> > +     Resource      *DomainResource    `xml:"resource"`
> >> > +     Devices       *DomainDeviceList  `xml:"devices"`
> >> > +     OS            *DomainOS          `xml:"os"`
> >> > +     SysInfo       *DomainSysInfo     `xml:"sysinfo"`
> >> > +     OnPoweroff    string             `xml:"on_poweroff,omitempty"`
> >> > +     OnReboot      string             `xml:"on_reboot,omitempty"`
> >> > +     OnCrash       string             `xml:"on_crash,omitempty"`
> >> > +     Features      *DomainFeatureList `xml:"features,omitempty"`
> >> >  }
> >>
> >> Regards,
> >> Daniel
> >> --
> >> |: https://berrange.com      -o-    https://www.flickr.com/photos/
> >> dberrange :|
> >> |: https://libvirt.org         -o-
> >> https://fstop138.berrange.com :|
> >> |: https://entangle-photo.org    -o-    https://www.instagram.com/dber
> >> range :|
> >
> >
> 
> 
> -- 
> *ry** isi*

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


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