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

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(a)redhat.com&gt; 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 :|