<div dir="ltr">Hi Daniel,<div> </div><div>I have addressed the issues you brought up in the commit titled 'remove superfluous state & omitempty entries' that has been posted to the list.</div><div> </div><div>--</div><div>cheers</div><div>ry</div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Apr 24, 2017 at 3:47 AM, Daniel P. Berrange <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. <div><div class="h5"> > +} > + > +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"` > +} </div></div>None of these should use 'DomainFeature', since they do not want a 'state' attribute. Also same note about omitempty not being needed. <div><div class="h5"> > + > 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"` > } </div></div>Regards, Daniel -- |: <a href="https://berrange.com" rel="noreferrer" target="_blank">https://berrange.com</a> -o- <a href="https://www.flickr.com/photos/dberrange" rel="noreferrer" target="_blank">https://www.flickr.com/photos/dberrange</a> :| |: <a href="https://libvirt.org" rel="noreferrer" target="_blank">https://libvirt.org</a> -o- <a href="https://fstop138.berrange.com" rel="noreferrer" target="_blank">https://fstop138.berrange.com</a> :| |: <a href="https://entangle-photo.org" rel="noreferrer" target="_blank">https://entangle-photo.org</a> -o- <a href="https://www.instagram.com/dberrange" rel="noreferrer" target="_blank">https://www.instagram.com/dberrange</a> :|</blockquote></div> </div></div>