Skip to content

Commit

Permalink
Fix crash for incompatible hubs
Browse files Browse the repository at this point in the history
  • Loading branch information
dorssel committed Jan 1, 2024
1 parent c76e3e7 commit bc752b8
Show file tree
Hide file tree
Showing 6 changed files with 137 additions and 73 deletions.
79 changes: 49 additions & 30 deletions UnitTests/BusId_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,25 @@ public void DefaultConstructor()

Assert.AreEqual(0, busId.Bus);
Assert.AreEqual(0, busId.Port);
Assert.IsTrue(busId.IsIncompatibleHub);
}

[TestMethod]
public void ConstructorWithInvalidBusThrows()
{
Assert.ThrowsException<ArgumentOutOfRangeException>(() =>
{
var busId = new BusId(0, 1);
});
}

[TestMethod]
public void ConstructorWithInvalidPortThrows()
{
Assert.ThrowsException<ArgumentOutOfRangeException>(() =>
{
var busId = new BusId(1, 0);
});
}

[TestMethod]
Expand All @@ -28,6 +47,15 @@ public void JsonConstructor()

Assert.AreEqual(testBus, busId.Bus);
Assert.AreEqual(testPort, busId.Port);
Assert.IsFalse(busId.IsIncompatibleHub);
}

[TestMethod]
public void ÍncompatibleHub()
{
Assert.AreEqual(0, BusId.IncompatibleHub.Bus);
Assert.AreEqual(0, BusId.IncompatibleHub.Port);
Assert.IsTrue(BusId.IncompatibleHub.IsIncompatibleHub);
}

sealed class BusIdData
Expand All @@ -43,10 +71,13 @@ sealed class BusIdData
"1-1 ",
"a-1",
"1-a",
"0-0",
"0-1",
"1-0",
"1-65536",
"65536-1",
"01-1",
"1-01",
];

public static IEnumerable<string[]> Invalid
Expand All @@ -55,6 +86,7 @@ public static IEnumerable<string[]> Invalid
}

static readonly string[] _Valid = [
"IncompatibleHub",
"1-1",
"1-2",
"1-65534",
Expand All @@ -73,31 +105,22 @@ public static IEnumerable<string[]> Invalid
"65535-65535",
];

public static IEnumerable<string[]> Valid
{
get => from value in _Valid select new string[] { value };
}
public static IEnumerable<string[]> Valid => from value in _Valid select new string[] { value };

static int ExpectedCompare(string left, string right)
{
var leftBus = ushort.Parse(left.Split('-')[0]);
var leftPort = ushort.Parse(left.Split('-')[1]);
var rightBus = ushort.Parse(right.Split('-')[0]);
var rightPort = ushort.Parse(right.Split('-')[1]);

return
leftBus < rightBus ? -1 :
leftBus > rightBus ? 1 :
leftPort < rightPort ? -1 :
leftPort > rightPort ? 1 : 0;
}
static int ExpectedCompare(string left, string right) =>
BusFromValidBusId(left) < BusFromValidBusId(right) ? -1 :
BusFromValidBusId(left) > BusFromValidBusId(right) ? 1 :
PortFromValidBusId(left) < PortFromValidBusId(right) ? -1 :
PortFromValidBusId(left) > PortFromValidBusId(right) ? 1 : 0;

public static IEnumerable<object[]> Compare
{
get => from left in _Valid from right in _Valid select new object[] { left, right, ExpectedCompare(left, right) };
}
public static IEnumerable<object[]> Compare => from left in _Valid from right in _Valid select new object[] { left, right, ExpectedCompare(left, right) };
}

static ushort BusFromValidBusId(string text) => (text == "IncompatibleHub") ? (ushort)0 : ushort.Parse(text.Split('-')[0]);

static ushort PortFromValidBusId(string text) => (text == "IncompatibleHub") ? (ushort)0 : ushort.Parse(text.Split('-')[1]);


[TestMethod]
[DynamicData(nameof(BusIdData.Invalid), typeof(BusIdData))]
public void TryParseInvalid(string text)
Expand All @@ -106,6 +129,7 @@ public void TryParseInvalid(string text)
Assert.IsFalse(result);
Assert.AreEqual(0, busId.Bus);
Assert.AreEqual(0, busId.Port);
Assert.IsTrue(busId.IsIncompatibleHub);
}

[TestMethod]
Expand All @@ -114,11 +138,8 @@ public void TryParseValid(string text)
{
var result = BusId.TryParse(text, out var busId);
Assert.IsTrue(result);

var expectedBus = ushort.Parse(text.Split('-')[0]);
var expectedPort = ushort.Parse(text.Split('-')[1]);
Assert.AreEqual(expectedBus, busId.Bus);
Assert.AreEqual(expectedPort, busId.Port);
Assert.AreEqual(BusFromValidBusId(text), busId.Bus);
Assert.AreEqual(PortFromValidBusId(text), busId.Port);
}

[TestMethod]
Expand All @@ -136,10 +157,8 @@ public void ParseInvalid(string text)
public void ParseValid(string text)
{
var busId = BusId.Parse(text);
var expectedBus = ushort.Parse(text.Split('-')[0]);
var expectedPort = ushort.Parse(text.Split('-')[1]);
Assert.AreEqual(expectedBus, busId.Bus);
Assert.AreEqual(expectedPort, busId.Port);
Assert.AreEqual(BusFromValidBusId(text), busId.Bus);
Assert.AreEqual(PortFromValidBusId(text), busId.Port);
}

[TestMethod]
Expand Down
55 changes: 42 additions & 13 deletions Usbipd.Automation/BusId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,32 +14,61 @@ public readonly record struct BusId
{
#if !NETSTANDARD
[JsonConstructor]
public BusId(ushort bus, ushort port) => (Bus, Port) = (bus, port);
#endif
public BusId(ushort bus, ushort port)
{
// Do not allow the explicit creation of the special IncompatibleHub value.
// Instead, use the static IncompatibleHub field (preferrable) or "default".
if (bus == 0)
{
throw new ArgumentOutOfRangeException(nameof(bus));
}
if (port == 0)
{
throw new ArgumentOutOfRangeException(nameof(port));
}
Bus = bus;
Port = port;
}

public ushort Bus { get; init; }
public ushort Port { get; init; }
public ushort Bus { get; }
public ushort Port { get; }

public override readonly string ToString() => $"{Bus}-{Port}";
/// <summary>
/// The special value 0-0 for hubs that do not expose a compatible busid.
/// NOTE: This is also the "default" value for this type.
/// </summary>
public static readonly BusId IncompatibleHub;

#if !NETSTANDARD
[JsonIgnore]
#endif
public bool IsIncompatibleHub => (Bus == 0) || (Port == 0);

public override readonly string ToString() => IsIncompatibleHub ? nameof(IncompatibleHub) : $"{Bus}-{Port}";

/// <summary>
/// NOTE: Valid inputs are x-y, where either x and y are between 1 and 65535, or both are 0.
/// NOTE: We do not allow leading zeros on non-zero values.
/// </summary>
public static bool TryParse(string input, out BusId busId)
{
// Must be 'x-y', where x and y are positive integers without leading zeros.
if (input == nameof(IncompatibleHub))
{
busId = IncompatibleHub;
return true;
}
var match = Regex.Match(input, "^([1-9][0-9]*)-([1-9][0-9]*)$");
if (match.Success
&& ushort.TryParse(match.Groups[1].Value, out var bus) && bus != 0
&& ushort.TryParse(match.Groups[2].Value, out var port) && port != 0)
&& ushort.TryParse(match.Groups[1].Value, out var bus)
&& ushort.TryParse(match.Groups[2].Value, out var port))
{
busId = new()
{
Bus = bus,
Port = port,
};
busId = new(bus, port);
return true;
}
else
{
busId = default;
busId = IncompatibleHub;
return false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion Usbipd.PowerShell/Installation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public static string ExePath
throw new ApplicationFailedException($"PowerShell module version {GitVersionInformation.MajorMinorPatch} does not match installed usbipd-win version {version}.");
}
#if DEBUG
return Path.GetFullPath(Path.Combine(Path.GetDirectoryName(System.Reflection.Assembly.GetCallingAssembly().Location), @"..\..\..\..\..\Usbipd\bin\x64\Debug\net7.0-windows8.0\win-x64\usbipd.exe"));
return Path.GetFullPath(Path.Combine(Path.GetDirectoryName(System.Reflection.Assembly.GetCallingAssembly().Location), @"..\..\..\..\Usbipd\bin\x64\Debug\net8.0-windows10.0.17763\win-x64\usbipd.exe"));
#else
return exeFile;
#endif
Expand Down
29 changes: 20 additions & 9 deletions Usbipd/CommandHandlers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,32 +36,39 @@ interface ICommandHandlers

sealed class CommandHandlers : ICommandHandlers
{
static IEnumerable<UsbDevice> GetDevicesByHardwareId(VidPid vidPid, bool connectedOnly, IConsole console)
static List<UsbDevice> GetDevicesByHardwareId(VidPid vidPid, bool connectedOnly, IConsole console)
{
if (!CheckNoStub(vidPid, console))
{
return [];
}
var filtered = new List<UsbDevice>();

Check warning on line 45 in Usbipd/CommandHandlers.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/CommandHandlers.cs#L45

Added line #L45 was not covered by tests
var devices = UsbDevice.GetAll().Where(d => (d.HardwareId == vidPid) && (!connectedOnly || d.BusId.HasValue));
var found = false;
foreach (var device in devices)
{
if (device.BusId.HasValue)
{
found = true;
console.ReportInfo($"Device with hardware-id '{vidPid}' found at busid '{device.BusId.Value}'.");
if (device.BusId.Value.IsIncompatibleHub)
{
console.ReportWarning($"Ignoring device with hardware-id '{vidPid}' connected to an incompatible hub.");

Check warning on line 53 in Usbipd/CommandHandlers.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/CommandHandlers.cs#L53

Added line #L53 was not covered by tests
}
else
{
console.ReportInfo($"Device with hardware-id '{vidPid}' found at busid '{device.BusId}'.");
filtered.Add(device);

Check warning on line 58 in Usbipd/CommandHandlers.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/CommandHandlers.cs#L57-L58

Added lines #L57 - L58 were not covered by tests
}
}
else if (device.Guid.HasValue)
{
found = true;
console.ReportInfo($"Persisted device with hardware-id '{vidPid}' found at guid '{device.Guid.Value:D}'.");
filtered.Add(device);

Check warning on line 64 in Usbipd/CommandHandlers.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/CommandHandlers.cs#L64

Added line #L64 was not covered by tests
}
}
if (!found)
if (filtered.Count == 0)
{
console.ReportError($"No devices found with hardware-id '{vidPid}'.");
}
return devices;
return filtered;

Check warning on line 71 in Usbipd/CommandHandlers.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/CommandHandlers.cs#L71

Added line #L71 was not covered by tests
}

static BusId? GetBusIdByHardwareId(VidPid vidPid, IConsole console)
Expand Down Expand Up @@ -148,12 +155,16 @@ Task<ExitCode> ICommandHandlers.List(bool usbids, IConsole console, Cancellation
{
state = device.IsForced ? "Shared (forced)" : "Shared";
}
else if (device.BusId.Value.IsIncompatibleHub)
{
state = "Incompatible hub";

Check warning on line 160 in Usbipd/CommandHandlers.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/CommandHandlers.cs#L160

Added line #L160 was not covered by tests
}
else
{
state = "Not shared";
}
// NOTE: Strictly speaking, both Bus and Port can be > 99. If you have one of those, you win a prize!
console.Write($"{device.BusId.Value,-5} ");
console.Write($"{(device.BusId.Value.IsIncompatibleHub ? string.Empty : device.BusId.Value),-5} ");
console.Write($"{device.HardwareId,-9} ");
console.WriteTruncated(GetDescription(device, usbids), 60, true);
console.WriteLine($" {state}");
Expand Down Expand Up @@ -498,7 +509,7 @@ Task<ExitCode> ICommandHandlers.Detach(BusId busId, IConsole console, Cancellati
Task<ExitCode> ICommandHandlers.Detach(VidPid vidPid, IConsole console, CancellationToken cancellationToken)
{
var devices = GetDevicesByHardwareId(vidPid, true, console);
if (!devices.Any())
if (devices.Count == 0)
{
// This would result in a no-op, which may not be what the user intended.
return Task.FromResult(ExitCode.Failure);
Expand Down
15 changes: 9 additions & 6 deletions Usbipd/ConfigurationManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -152,14 +152,17 @@ public static BusId GetBusId(uint deviceNode)
var match = LocationInfoRegex().Match(locationInfo);
if (!match.Success)
{
// We want users to report this one.
throw new NotSupportedException($"DEVPKEY_Device_LocationInfo returned '{locationInfo}', expected form 'Port_#0123.Hub_#4567'");
// This is probably a device on an unsupported hub-type.
// See for example https://github.com/dorssel/usbipd-win/issues/809.
return BusId.IncompatibleHub;

Check warning on line 157 in Usbipd/ConfigurationManager.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/ConfigurationManager.cs#L157

Added line #L157 was not covered by tests
}
return new()
var bus = ushort.Parse(match.Groups[2].Value, CultureInfo.InvariantCulture);
var port = ushort.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture);

Check warning on line 160 in Usbipd/ConfigurationManager.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/ConfigurationManager.cs#L159-L160

Added lines #L159 - L160 were not covered by tests
if (bus == 0 || port == 0)
{
Bus = ushort.Parse(match.Groups[2].Value, CultureInfo.InvariantCulture),
Port = ushort.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture),
};
return BusId.IncompatibleHub;

Check warning on line 163 in Usbipd/ConfigurationManager.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/ConfigurationManager.cs#L163

Added line #L163 was not covered by tests
}
return new(bus, port);

Check warning on line 165 in Usbipd/ConfigurationManager.cs

View check run for this annotation

Codecov / codecov/patch

Usbipd/ConfigurationManager.cs#L165

Added line #L165 was not covered by tests
}

public static BusId? GetBusId(string instanceId)
Expand Down
Loading

0 comments on commit bc752b8

Please sign in to comment.