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/dberrange :|