<div dir="ltr">Hi Daniel!<div> </div><div>Thank you for review, you're absolutely right about omitempty on structures, sorry for that. Will rename DomainEntry as well.</div></div><div class="gmail_extra"> <div class="gmail_quote">On Thu, Jan 5, 2017 at 1:58 PM, 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"><div class="HOEnZb"><div class="h5">On Thu, Jan 05, 2017 at 09:54:25AM +0100, Alexey Slaykovsky wrote: > Signed-off-by: Alexey Slaykovsky <<a href="mailto:aslaikov@redhat.com">aslaikov@redhat.com</a>> > --- > 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"` > +} </div></div>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 -- |: <a href="http://berrange.com" rel="noreferrer" target="_blank">http://berrange.com</a> -o- <a href="http://www.flickr.com/photos/dberrange/" rel="noreferrer" target="_blank">http://www.flickr.com/photos/dberrange/</a> :| |: <a href="http://libvirt.org" rel="noreferrer" target="_blank">http://libvirt.org</a> -o- <a href="http://virt-manager.org" rel="noreferrer" target="_blank">http://virt-manager.org</a> :| |: <a href="http://entangle-photo.org" rel="noreferrer" target="_blank">http://entangle-photo.org</a> -o- <a href="http://search.cpan.org/~danberr/" rel="noreferrer" target="_blank">http://search.cpan.org/~danberr/</a> :| </blockquote></div> </div>