From e840be9d0654eb9374b0a1064a4d033ff5d835e1 Mon Sep 17 00:00:00 2001 From: Sebastian Zagrodzki Date: Fri, 13 Sep 2024 20:43:01 +0200 Subject: [PATCH] Do not assume interface numbers follow the slice indices. (#134) Do not assume interface numbers follow the slice indices. This is a continuation of https://github.com/google/gousb/commit/9ad54830f48186bf46677a3e4aec6667b1462ebb which tried to solve the problem of non-contiguous interface indices; this commit modifies another code path that had the same assumption. --- config.go | 28 +++++++++------------ device.go | 8 ++++-- device_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++--- interface.go | 11 +++++++++ 4 files changed, 91 insertions(+), 22 deletions(-) diff --git a/config.go b/config.go index 3c53510..1b5111d 100644 --- a/config.go +++ b/config.go @@ -49,25 +49,17 @@ func (c ConfigDesc) String() string { return fmt.Sprintf("Configuration %d", c.Number) } -func (c ConfigDesc) intfDesc(num, alt int) (*InterfaceSetting, error) { +func (c ConfigDesc) intfDesc(num int) (*InterfaceDesc, error) { // In an ideal world, interfaces in the descriptor would be numbered // contiguously starting from 0, as required by the specification. In the // real world however the specification is sometimes ignored: // https://github.com/google/gousb/issues/65 ifs := make([]int, len(c.Interfaces)) - for i := range c.Interfaces { - ifs[i] = c.Interfaces[i].Number - if ifs[i] != num { - continue + for i, desc := range c.Interfaces { + if desc.Number == num { + return &desc, nil } - alts := make([]int, len(c.Interfaces[i].AltSettings)) - for a := range c.Interfaces[i].AltSettings { - alts[a] = c.Interfaces[i].AltSettings[a].Alternate - if alts[a] == alt { - return &c.Interfaces[i].AltSettings[a], nil - } - } - return nil, fmt.Errorf("alternate setting %d not found for %s, available alt settings: %v", alt, c.Interfaces[i], alts) + ifs[i] = desc.Number } return nil, fmt.Errorf("interface %d not found, available interface numbers: %v", num, ifs) } @@ -120,9 +112,13 @@ func (c *Config) Interface(num, alt int) (*Interface, error) { return nil, fmt.Errorf("Interface(%d, %d) called on %s after Close", num, alt, c) } - altInfo, err := c.Desc.intfDesc(num, alt) + intf, err := c.Desc.intfDesc(num) if err != nil { - return nil, fmt.Errorf("descriptor of interface (%d, %d) in %s: %v", num, alt, c, err) + return nil, fmt.Errorf("descriptor of interface %d in %s: %v", num, c, err) + } + altInfo, err := intf.altSetting(alt) + if err != nil { + return nil, fmt.Errorf("descriptor of alternate setting %d of interface %d in %s: %v", alt, num, c, err) } c.mu.Lock() @@ -137,7 +133,7 @@ func (c *Config) Interface(num, alt int) (*Interface, error) { } // Select an alternate setting if needed (device has multiple alternate settings). - if len(c.Desc.Interfaces[num].AltSettings) > 1 { + if len(intf.AltSettings) > 1 { if err := c.dev.ctx.libusb.setAlt(c.dev.handle, uint8(num), uint8(alt)); err != nil { c.dev.ctx.libusb.release(c.dev.handle, uint8(num)) return nil, fmt.Errorf("failed to set alternate config %d on interface %d of %s: %v", alt, num, c, err) diff --git a/device.go b/device.go index 4180b11..49c1f31 100644 --- a/device.go +++ b/device.go @@ -276,9 +276,13 @@ func (d *Device) InterfaceDescription(cfgNum, intfNum, altNum int) (string, erro if err != nil { return "", fmt.Errorf("%s: %v", d, err) } - alt, err := cfg.intfDesc(intfNum, altNum) + intf, err := cfg.intfDesc(intfNum) if err != nil { - return "", fmt.Errorf("%s, configuration %d: %v", d, cfgNum, err) + return "", fmt.Errorf("%s, configuration %d interface %d: %v", d, cfgNum, intfNum, err) + } + alt, err := intf.altSetting(altNum) + if err != nil { + return "", fmt.Errorf("%s, configuration %d interface %d alternate setting %d: %v", d, cfgNum, intfNum, altNum, err) } return d.GetStringDescriptor(alt.iInterface) } diff --git a/device_test.go b/device_test.go index 4710a8a..a394d21 100644 --- a/device_test.go +++ b/device_test.go @@ -117,18 +117,18 @@ func TestClaimAndRelease(t *testing.T) { t.Errorf("%s.InEndpoint(%d): got %+v, want %+v", intf, ep1Addr, got, want) } - if _, err := cfg.Interface(1, 0); err == nil { + if _, err := cfg.Interface(if1Num, 0); err == nil { t.Fatalf("%s.Interface(1, 0): got nil, want non nil, because Interface 1 is already claimed.", cfg) } // intf2 is interface #0, not claimed yet. - intf2, err := cfg.Interface(0, 0) + intf2, err := cfg.Interface(if2Num, alt2Num) if err != nil { t.Fatalf("%s.Interface(0, 0): got %v, want nil", cfg, err) } if err := cfg.Close(); err == nil { - t.Fatalf("%s.Close(): got nil, want non nil, because the Interface was not released.", cfg) + t.Fatalf("%s.Close(): got nil, want non nil, because the Interfaces #1/2 was not released.", cfg) } if err := dev.Close(); err == nil { t.Fatalf("%s.Close(): got nil, want non nil, because the Config was not released.", cfg) @@ -136,7 +136,7 @@ func TestClaimAndRelease(t *testing.T) { intf.Close() if err := cfg.Close(); err == nil { - t.Fatalf("%s.Close(): got nil, want non nil, because the Interface was not released.", cfg) + t.Fatalf("%s.Close(): got nil, want non nil, because the Interface #2 was not released.", cfg) } if err := dev.Close(); err == nil { t.Fatalf("%s.Close(): got nil, want non nil, because the Config was not released.", dev) @@ -244,3 +244,61 @@ func TestAutoDetachFailure(t *testing.T) { t.Fatalf("%s.Config(1) got nil, but want no nil because interface fails to detach", dev) } } + +func TestInterface(t *testing.T) { + t.Parallel() + c := newContextWithImpl(newFakeLibusb()) + defer func() { + if err := c.Close(); err != nil { + t.Errorf("Context.Close: %v", err) + } + }() + + for _, tc := range []struct { + name string + vid, pid ID + cfgNum, ifNum, altNum int + }{{ + name: "simple", + vid: 0x8888, + pid: 0x0002, + cfgNum: 1, + ifNum: 0, + altNum: 0, + }, { + name: "alt_setting", + vid: 0x8888, + pid: 0x0002, + cfgNum: 1, + ifNum: 1, + altNum: 1, + }, { + name: "noncontiguous_interfaces", + vid: 0x8888, + pid: 0x0002, + cfgNum: 1, + ifNum: 3, + altNum: 2, + }} { + t.Run(tc.name, func(t *testing.T) { + dev, err := c.OpenDeviceWithVIDPID(0x8888, 0x0002) + if err != nil { + t.Fatalf("OpenDeviceWithVIDPID(0x8888, 0x0002): %v", err) + } + if dev == nil { + t.Fatal("OpenDeviceWithVIDPID(0x8888, 0x0002): got nil device, need non-nil") + } + defer dev.Close() + cfg, err := dev.Config(tc.cfgNum) + if err != nil { + t.Fatalf("%s.Config(%d): %v", dev, tc.cfgNum, err) + } + defer cfg.Close() + intf, err := cfg.Interface(tc.ifNum, tc.altNum) + if err != nil { + t.Fatalf("%s.Interface(%d, %d): %v", cfg, tc.ifNum, tc.altNum, err) + } + intf.Close() + }) + } +} diff --git a/interface.go b/interface.go index 0af77a9..661dfc5 100644 --- a/interface.go +++ b/interface.go @@ -29,6 +29,17 @@ type InterfaceDesc struct { AltSettings []InterfaceSetting } +func (i *InterfaceDesc) altSetting(alt int) (*InterfaceSetting, error) { + alts := make([]int, len(i.AltSettings)) + for a, s := range i.AltSettings { + if s.Alternate == alt { + return &s, nil + } + alts[a] = s.Alternate + } + return nil, fmt.Errorf("alternate setting %d not found for %s, available alt settings: %v", alt, i, alts) +} + // String returns a human-readable description of the interface descriptor and // its alternate settings. func (i InterfaceDesc) String() string {