Re: [PATCH go-xml] Added domain OS struct and tests

[Aleksei Slaikovskii <aslaikov@xxxxxxxxxx>]

Hi Daniel!

Thank you for review, you're absolutely right about omitempty on structures, sorry for that. Will rename DomainEntry as well.

On Thu, Jan 5, 2017 at 1:58 PM, Daniel P. Berrange <berrange@xxxxxxxxxx> wrote:

On Thu, Jan 05, 2017 at 09:54:25AM +0100, Alexey Slaykovsky wrote:
> Signed-off-by: Alexey Slaykovsky <aslaikov@xxxxxxxxxx>
> ---
>  domain.go      |  68 ++++++++++++++++++++++++
>  domain_test.go | 164 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 232 insertions(+)
>
> diff --git a/domain.go b/domain.go
> index c98908e..25155ee 100644
> --- a/domain.go
> +++ b/domain.go
> @@ -126,6 +126,69 @@ type DomainMemory struct {
>       Unit  string `xml:"unit,attr"`
>  }
>
> +type DomainOSType struct {
> +     Arch    string `xml:"arch,attr"`
> +     Machine string `xml:"machine,attr"`
> +     Type    string `xml:",chardata"`
> +}
> +
> +type DomainSMBios struct {
> +     Mode string `xml:"mode,attr"`
> +}
> +
> +type DomainNVRam struct {
> +     NVRam    string `xml:",chardata"`
> +     Template string `xml:"template,attr,omitempty"`
> +}
> +
> +type DomainBootDevice struct {
> +     Dev string `xml:"dev,attr"`
> +}
> +
> +type DomainBootMenu struct {
> +     Enabled string `xml:"enabled,attr"`
> +     Timeout string `xml:"timeout,attr,omitempty"`
> +}
> +
> +type DomainSysInfo struct {
> +     Type      string        `xml:"type,attr"`
> +     System    []DomainEntry `xml:"system>entry"`
> +     BIOS      []DomainEntry `xml:"bios>entry"`
> +     BaseBoard []DomainEntry `xml:"baseBoard>entry"`
> +}
> +
> +type DomainEntry struct {
> +     Name  string `xml:"name,attr"`
> +     Value string `xml:",chardata"`
> +}

Can you call this "DomainSysInfoEntry" so its more tightly scoped.

> +
> +type DomainBIOS struct {
> +     UseSerial     string `xml:"useserial,attr"`
> +     RebootTimeout string `xml:"rebootTimeout,attr"`
> +}
> +
> +type DomainLoader struct {
> +     Path     string `xml:",chardata"`
> +     Readonly string `xml:"readonly,attr"`
> +     Secure   string `xml:"secure,attr"`
> +     Type     string `xml:"type,attr"`
> +}
> +
> +type DomainOS struct {
> +     Type        *DomainOSType      `xml:"type"`
> +     Loader      *DomainLoader      `xml:"loader,omitempty"`
> +     NVRam       *DomainNVRam       `xml:"nvram"`

I'm curious why we need 'omitempty' on "Loader" and a few
others below, but don't need it on "NVRam". Based on your
test case it seems the formatter omits NVRam automatically
when it is nil, which would suggest we don't need omitempty
on the other struct fields either - just need it on the
string fields.

> +     Kernel      string             `xml:"kernel,omitempty"`
> +     Initrd      string             `xml:"initrd,omitempty"`
> +     KernelArgs  string             `xml:"cmdline,omitempty"`
> +     BootDevices []DomainBootDevice `xml:"boot"`
> +     BootMenu    *DomainBootMenu    `xml:"bootmenu,omitempty"`
> +     SMBios      *DomainSMBios      `xml:"smbios,omitempty"`
> +     BIOS        *DomainBIOS        `xml:"bios,omitempty"`
> +     Init        string             `xml:"init,omitempty"`
> +     InitArgs    []string           `xml:"initarg,omitempty"`
> +}

Looks good apart from this two nitpicks.

Regards,
Daniel
--
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list