Fix leaking udev reference in LinuxUsbDevice

[mattmacleod]

This change fixes a leaking device reference in the Linux USB device implementation. When an interface device is encountered (i.e. devtype is "usb_device") the rest of the loop was skipped and the allocated device would never be unreferenced. This change adds an additional explicit unreference in this case.

This was the underlying leak causing the issue reported by @botto in #1670

[dbwiddis]

Gah, good find. I should have seen that when looking at #1670.

Your fix doesn't handle the case where udev_device_new_from_syspath returns null (which would be caught in the existing conditional check) and you'd try to unref that. I suppose the native code wouldn't care but it's still somewhat sloppy.

This whole class could use a refactoring, if you wouldn't mind putting in a bit more work here:

• I'd prefer to use a try - finally approach to defensively handle references with exceptions (and continue and return)
• I wrote this code 5 years ago and since then I've contributed a more object-oriented front end to JNA. See the LinuxHWDiskStore class getDisks() method, code starting at line 168.

At a minimum please change your PR to do something like this pattern:

UdevDevice dev = Udev.INSTANCE.udev_device_new_from_syspath(udev, path);
if (dev != null) {
    try {
        // existing code between new and unref
    } finally {
        Udev.INSTANCE.udev_device_unref(dev);
    }
}

However, if you can refactor to the example in LinuxHWDiskStore it'd be even better.

[mattmacleod]

Thanks for the pointers @dbwiddis – I have updated the PR to reflect the approach used in LinuxHWDiskStore as suggested. Confirmed that this still functions as expected in my use-case, and that it doesn't leak any more. Please let me know if you'd like any tweaks.

[dbwiddis]

Thanks! This looks great and I'll go ahead and merge it.

[dbwiddis]

Looking at this again, I see that it's possible to return null if the native call fails, but of the four times we call this, we only null check one of them. No need to change this PR for that but I'll have to think about whether we should update all of them.

[dbwiddis]

Similar to udev_new this can theoretically return null but probably never will. Something to think about for a future fix.