[Kde-hardware-devel] Review Request: HAL: Make devicesFromQuery more similar to the UDisks implementation.

[Alberto Villa]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105615/#review16121
-----------------------------------------------------------


One issue and one possible improvement, in my opinion.

Currently, when parentUdi is set and type is Unknown, all the children are returned. In your patch, none of the children are returned, unless the last routine in queryDeviceInterface() returns true with Unknown type, but I doubt it as it's optimised for types like Generic, etc., but not for Unknown. Anyway, it's a case we should optimise here.
Should we think that the current behaviour is wrong, then why is allDevices() returned when both parentUdi and type aren't specified?

About the improvement, I'm speaking of removing duplication. The first two checks are mostly equal, they could be condensed into one (tweaking the if() inside the foreach()), with "return allDevices()" being moved on top if (parentUdi.isEmpty() and type == Unknown).

- Alberto Villa


On July 19, 2012, 2:17 a.m., Raphael Kubo da Costa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105615/
> -----------------------------------------------------------
> 
> (Updated July 19, 2012, 2:17 a.m.)
> 
> 
> Review request for Solid and Alberto Villa.
> 
> 
> Description
> -------
> 
> HAL: Make devicesFromQuery more similar to the UDisks implementation.
> 
> The devicesFromQuery implementation in the UDisks backend is saner, as
> it simply relays the required checks for validity to each Device we
> are interested in.
> 
> So far, HalDevice::queryDeviceInterface() performed some checks
> depending on the DeviceInterface type being passed to it, while
> HalManager::devicesFromQuery() did not filter the results it got in
> the same way. What's more, some checks such as the video4linux ones
> were being made in both places, leading to some needless duplication
> in functionality.
> 
> As a side-effect, this fixes a bug made visibile by a recent commit to
> libktorrent: retrieving StorageAccess devices with listFromType()
> would simply query HAL for devices with the "volume" capability (which
> includes swap partitions and other non-mountable things), so using
> Device::asDeviceInterface(Solid::DeviceInterface::StorageAccess) would
> sometimes return 0 on a few of those entries retrieved earlier.
> 
> 
> Diffs
> -----
> 
>   solid/solid/backends/hal/halmanager.h ec42fac1d2b5dc306e9b8e00432bcbe5854a6fb9 
>   solid/solid/backends/hal/halmanager.cpp 2c9c6c0c0a8385bee37ce77d488e0395f2f90a06 
> 
> Diff: http://git.reviewboard.kde.org/r/105615/diff/
> 
> 
> Testing
> -------
> 
> libktorrent stopped crashing on bt::MountPoint()
> 
> 
> Thanks,
> 
> Raphael Kubo da Costa
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20120719/37174cec/attachment.html>