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

ping, ref < https://www.redhat.com/archives/libvir-list/2017-April/msg01073.html>

On Thu, Apr 27, 2017 at 12:35 PM, Ryan Goodfellow <rgoodfel(a)isi.edu&gt; 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(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 :| > > -- *ry**@isi*