[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH go-xml] Add support for Node Device with basic testing.



Thanks for the review.

On Tue, May 30, 2017 at 8:46 AM, Daniel P. Berrange <berrange redhat com> wrote:
> On Mon, May 29, 2017 at 03:58:52AM -0400, Vladik Romanovsky wrote:
>> ---
>>  node_device.go      | 277 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  node_device_test.go | 111 +++++++++++++++++++++
>>  2 files changed, 388 insertions(+)
>>  create mode 100644 node_device.go
>>  create mode 100644 node_device_test.go
>>
>> diff --git a/node_device.go b/node_device.go
>> new file mode 100644
>> index 0000000..5375d32
>> --- /dev/null
>> +++ b/node_device.go
>> @@ -0,0 +1,277 @@
>> +/*
>> + * This file is part of the libvirt-go-xml project
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + *
>> + * Copyright (C) 2017 Red Hat, Inc.
>> + *
>> + */
>> +
>> +package libvirtxml
>> +
>> +import (
>> +     "encoding/xml"
>> +     "fmt"
>> +)
>> +
>> +type NodeDevice struct {
>> +     Name       string          `xml:"name"`
>> +     Path       string          `xml:"path,omitempty"`
>> +     Parent     string          `xml:"parent,omitempty"`
>> +     Driver     string          `xml:"driver>name,omitempty"`
>> +     Capability *CapabilityType `xml:"capability"`
>> +}
>
> A single device can have multiple capabilities.
Do you mean it should be:
             Capability []CapabilityType `xml:"capability"` ?

>
>> +
>> +type CapabilityType struct {
>> +     Type interface{} `xml:"type,attr"`
>> +}
>
> I'm not convinced about this approach - it differs from what we've
> done in other schemas, where we just have a single struct containing
> the union of data from all types. The user has no idea what they
> should be providing for 'Type' here since its a generic interface
> type.
>
Yes. I wasn't sure about this either. Originally, I wanted to avoid
the Type attribute and make
CapabilityType to be an interface type.
However, I figured that it's not possible to write an UnmarshalXML for
an interface type.

The reason I didn't use the same approach as in the rest of the
schemas is because the Capability
determins it's type by a XML attribute and not an element name.
I didn't find an easy way to query the xml attribue as something
like.. `xml:"type=something,attr"`

OK, I'll try a ddiffernt approach.

>>
>> +type IDName struct {
>> +     ID   string `xml:"id,attr"`
>> +     Name string `xml:",chardata"`
>> +}
>> +
>> +type PciExpress struct {
>> +     Links []PciExpressLink `xml:"link"`
>> +}
>> +
>> +type PciExpressLink struct {
>> +     Validity string  `xml:"validity,attr,omitempty"`
>> +     Speed    float64 `xml:"speed,attr,omitempty"`
>> +     Port     int     `xml:"port,attr,omitempty"`
>> +     Width    int     `xml:"width,attr,omitempty"`
>> +}
>> +
>> +type IOMMUGroupType struct {
>> +     Number int `xml:"number,attr"`
>> +}
>> +
>> +type NUMA struct {
>> +     Node int `xml:"node,attr"`
>> +}
>> +
>> +type PciCapabilityType struct {
>> +     Domain       int             `xml:"domain,omitempty"`
>> +     Bus          int             `xml:"bus,omitempty"`
>> +     Slot         int             `xml:"slot,omitempty"`
>> +     Function     int             `xml:"function,omitempty"`
>> +     Product      IDName          `xml:"product,omitempty"`
>> +     Vendor       IDName          `xml:"vendor,omitempty"`
>> +     IommuGroup   IOMMUGroupType  `xml:"iommuGroup,omitempty"`
>> +     Numa         NUMA            `xml:"numa,omitempty"`
>> +     PciExpress   PciExpress      `xml:"pci-express,omitempty"`
>> +     Capabilities []PciCapability `xml:"capability,omitempty"`
>> +}
>> +
>> +type PCIAddress struct {
>> +     Domain   string `xml:"domain,attr"`
>> +     Bus      string `xml:"bus,attr"`
>> +     Slot     string `xml:"slot,attr"`
>> +     Function string `xml:"function,attr"`
>> +}
>> +
>> +type PciCapability struct {
>
> There's alot of inconsistency in capitalization of abbreviations
> in this file.  PCI vs Pci,  Numa vs NUMA, IOMMU vs Iiommu. They
> should generally always be capitalized.
>
>> +     Type     string`xml:"type,attr"`
>> +     Address  []PCIAddress `xml:"address,omitempty"`
>> +     MaxCount int          `xml:"maxCount,attr,omitempty"`
>> +}
>> +
>> +type SystemHardware struct {
>> +     Vendor  string `xml:"vendor"`
>> +     Version string `xml:"version"`
>> +     Serial  string `xml:"serial"`
>> +     UUID    string `xml:"uuid"`
>> +}
>> +
>> +type SystemFirmware struct {
>> +     Vendor      string `xml:"vendor"`
>> +     Version     string `xml:"version"`
>> +     ReleaseData string `xml:"release_date"`
>> +}
>> +
>> +type SystemCapabilityType struct {
>> +     Product  string         `xml:"product"`
>> +     Hardware SystemHardware `xml:"hardware"`
>> +     Firmware SystemFirmware `xml:"firmware"`
>> +}
>> +
>> +type USBDeviceCapabilityType struct {
>> +     Bus     int    `xml:"bus"`
>> +     Device  int    `xml:"device"`
>> +     Product IDName `xml:"product,omitempty"`
>> +     Vendor  IDName `xml:"vendor,omitempty"`
>> +}
>> +
>> +type USBCapabilityType struct {
>> +     Number      int    `xml:"number"`
>> +     Class       int    `xml:"class"`
>> +     Subclass    int    `xml:"subclass"`
>> +     Protocol    int    `xml:"protocol"`
>> +     Description string `xml:"description,omitempty"`
>> +}
>> +
>> +type NetOffloadFeatures struct {
>> +     Name string `xml:"number"`
>> +}
>> +
>> +type NetLink struct {
>> +     State string `xml:"state,attr"`
>> +     Speed string `xml:"speed,attr,omitempty"`
>> +}
>> +
>> +type NetCapability struct {
>> +     Type string `xml:"type,attr"`
>> +}
>> +
>> +type NetCapabilityType struct {
>> +     Interface  string               `xml:"interface"`
>> +     Address    string               `xml:"address"`
>> +     Link       NetLink              `xml:"link"`
>> +     Features   []NetOffloadFeatures `xml:"feature,omitempty"`
>> +     Capability NetCapability        `xml:"capability"`
>> +}
>> +
>> +type SCSIVportsOPS struct {
>> +     Vports    int `xml:"vports,omitempty"`
>> +     MaxVports int `xml:"maxvports,,omitempty"`
>> +}
>> +
>> +type SCSIFCHost struct {
>> +     WWNN      string `xml:"wwnn,omitempty"`
>> +     WWPN      string `xml:"wwpn,omitempty"`
>> +     FabricWWN string `xml:"fabric_wwn,omitempty"`
>> +}
>> +
>> +type SCSIHostCapability struct {
>> +     VportsOPS SCSIVportsOPS `xml:"vports_ops"`
>> +     FCHost    SCSIFCHost    `xml:"fc_host"`
>> +}
>> +
>> +type SCSIHostCapabilityType struct {
>> +     Host       int                `xml:"host"`
>> +     UniqueID   int                `xml:"unique_id"`
>> +     Capability SCSIHostCapability `xml:"capability"`
>> +}
>> +
>> +type SCSICapabilityType struct {
>> +     Host   int    `xml:"host"`
>> +     Bus    int    `xml:"bus"`
>> +     Target int    `xml:"target"`
>> +     Lun    int    `xml:"lun"`
>> +     Type   string `xml:"type"`
>> +}
>> +
>> +type StroageCap struct {
>
> s/Stroage/Storage/
>
>> +     Type           string `xml:"match,attr"`
>> +     MediaAvailable int    `xml:"media_available,omitempty"`
>> +     MediaSize      int    `xml:"media_size,omitempty"`
>> +     MediaLable     int    `xml:"media_label,omitempty"`
>> +}
>> +
>> +type StorageCapabilityType struct {
>> +     Block        string     `xml:"block"`
>> +     Bus          string     `xml:"bus"`
>> +     DriverType   string     `xml:"drive_type"`
>> +     Model        string     `xml:"model"`
>> +     Vendor       string     `xml:"vendor"`
>> +     Serial       string     `xml:"serial"`
>> +     Size         int        `xml:"size"`
>> +     Capatibility StroageCap `xml:"capability,omitempty"`
>> +}
>> +
>> +type DRMCapabilityType struct {
>> +     Type string `xml:"type"`
>> +}
>
> Note that every struct is in the same 'libvirtxml' namespace, even if the
> declarations are spread across different .go files. We need the structs
> for each XML document to be isolated from each other, so you need to have
> a 'NodeDevice' prefix on all these struct names.
>
>
>> +
>> +func (c *CapabilityType) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
>> +     for _, attr := range start.Attr {
>> +             fmt.Println(attr.Name.Local)
>> +             if attr.Name.Local == "type" {
>> +                     switch attr.Value {
>> +                     case "pci":
>> +                             var pciCaps PciCapabilityType
>> +                             if err := d.DecodeElement(&pciCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = pciCaps
>> +                     case "system":
>> +                             var systemCaps SystemCapabilityType
>> +                             if err := d.DecodeElement(&systemCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = systemCaps
>> +                     case "usb_device":
>> +                             var usbdevCaps USBDeviceCapabilityType
>> +                             if err := d.DecodeElement(&usbdevCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = usbdevCaps
>> +                     case "usb":
>> +                             var usbCaps USBCapabilityType
>> +                             if err := d.DecodeElement(&usbCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = usbCaps
>> +                     case "net":
>> +                             var netCaps NetCapabilityType
>> +                             if err := d.DecodeElement(&netCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = netCaps
>> +                     case "scsi_host":
>> +                             var scsiHostCaps SCSIHostCapabilityType
>> +                             if err := d.DecodeElement(&scsiHostCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = scsiHostCaps
>> +                     case "scsi":
>> +                             var scsiCaps SCSICapabilityType
>> +                             if err := d.DecodeElement(&scsiCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = scsiCaps
>> +                     case "storage":
>> +                             var storageCaps StorageCapabilityType
>> +                             if err := d.DecodeElement(&storageCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = storageCaps
>> +                     case "drm":
>> +                             var drmCaps DRMCapabilityType
>> +                             if err := d.DecodeElement(&drmCaps, &start); err != nil {
>> +                                     return err
>> +                             }
>> +                             c.Type = drmCaps
>> +                     }
>> +             }
>> +     }
>> +     return nil
>> +}
>> +
>> +func (c *NodeDevice) Unmarshal(doc string) error {
>> +     return xml.Unmarshal([]byte(doc), c)
>> +}
>> +
>> +func (c *NodeDevice) Marshal() (string, error) {
>> +     doc, err := xml.MarshalIndent(c, "", "  ")
>> +     if err != nil {
>> +             return "", err
>> +     }
>> +     return string(doc), nil
>> +}
>> diff --git a/node_device_test.go b/node_device_test.go
>> new file mode 100644
>> index 0000000..129956b
>> --- /dev/null
>> +++ b/node_device_test.go
>> @@ -0,0 +1,111 @@
>> +/*
>> + * This file is part of the libvirt-go-xml project
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + *
>> + * Copyright (C) 2017 Red Hat, Inc.
>> + *
>> + */
>> +
>> +package libvirtxml
>> +
>> +import (
>> +     "reflect"
>> +     "strings"
>> +     "testing"
>> +)
>> +
>> +var NodeDeviceTestData = []struct {
>> +     Object *NodeDevice
>> +     XML    []string
>> +}{
>> +     {
>> +             Object: &NodeDevice{
>> +                     Name:   "pci_0000_81_00_0",
>> +                     Parent: "pci_0000_80_01_0",
>> +                     Driver: "ixgbe",
>> +                     Capability: &CapabilityType{
>> +                             Type: PciCapabilityType{
>> +                                     Domain:   1,
>> +                                     Bus:      21,
>> +                                     Slot:     10,
>> +                                     Function: 50,
>> +                                     Product: IDName{
>> +                                             ID:   "0x1528",
>> +                                             Name: "Ethernet Controller 10-Gigabit X540-AT2",
>> +                                     },
>> +                                     Vendor: IDName{
>> +                                             ID:   "0x8086",
>> +                                             Name: "Intel Corporation",
>> +                                     },
>> +                                     IommuGroup: IOMMUGroupType{
>> +                                             Number: 3,
>> +                                     },
>> +                                     Numa: NUMA{
>> +                                             Node: 1,
>> +                                     },
>> +                                     Capabilities: []PciCapability{
>> +                                             PciCapability{
>> +                                                     Type:     "virt_functions",
>> +                                                     MaxCount: 63,
>> +                                             },
>> +                                     },
>> +                             },
>> +                     },
>> +             },
>> +             XML: []string{
>> +                     `<device>`,
>> +                     `  <name>pci_0000_81_00_0</name>`,
>> +                     `  <parent>pci_0000_80_01_0</parent>`,
>> +                     `  <driver>`,
>> +                     `               <name>ixgbe</name>`,
>> +                     `  </driver>`,
>> +                     `  <capability type='pci'>`,
>> +                     `       <domain>1</domain>`,
>> +                     `       <bus>21</bus>`,
>> +                     `       <slot>10</slot>`,
>> +                     `       <function>50</function>`,
>> +                     `       <product id='0x1528'>Ethernet Controller 10-Gigabit X540-AT2</product>`,
>> +                     `       <vendor id='0x8086'>Intel Corporation</vendor>`,
>> +                     `       <capability type='virt_functions' maxCount='63'/>`,
>> +                     `       <iommuGroup number='3'>`,
>> +                     `         <address domain='0x0000' bus='0x15' slot='0x00' function='0x4'/>`,
>> +                     `       </iommuGroup>`,
>> +                     `   <numa node='1'/>`,
>> +                     `  </capability>`,
>> +                     `</device>`,
>> +             },
>> +     },
>> +}
>> +
>> +func TestNodeDevice(t *testing.T) {
>> +     for _, test := range NodeDeviceTestData {
>> +             xmlDoc := strings.Join(test.XML, "\n")
>> +             nodeDevice := NodeDevice{}
>> +             err := nodeDevice.Unmarshal(xmlDoc)
>> +             if err != nil {
>> +                     t.Fatal(err)
>> +             }
>> +
>> +             res := reflect.DeepEqual(&nodeDevice, test.Object)
>> +             if !res {
>> +                     t.Fatal("Bad NodeDevice object creation.")
>> +             }
>> +     }
>> +}
>> --
>> 2.9.4
>>
>> --
>> libvir-list mailing list
>> libvir-list redhat com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
> 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]