On Mon, 2017-05-29 at 22:01 +0200, Daniel Wagner wrote:
On 05/26/2017 12:14 PM, Patrik Flykt wrote:
> Blacklisting does not work when devices change their interface
> but this patch set attempts to fix that. With the following changes
> blacklisting the first interface name should make ConnMan aware of
> the interface when it is renamed and vice versa.
> In patch 1/4 the functionality provided by detect.c is merged with
> device.c. The code in detect.c does nothing else, so this
> dependencies between the files.
> Patch 2/4 removes the additional blacklisting done by rtnl.c. This
> additional check will set the device type to unknown, but after
> the device may get its device and service types set to correct
> again. This is confusing and will confuse blacklisting later on.
> Patch 3/4 adds interface names to newlink and dellink callbacks. As
> the only callbacks are registered for device.c during
> time, it does not matter that trigger_rtnl passes NULL as interface
> name as there will not be any ipdevices at that time.
> Patch 4/4 moves the interface blacklist checking to the RTNL
> callback as this callback is the only function in ConnMan that uses
> connman_device_create_from_index where the check was located
> With this the new name of the interface can be checked and the
> unregistered should the blacklisting entry be only for the renamed
> device name.
> As this is a bit difficult to test, I hope to have somebody running
> this during their builds. But if someone feels like it, please
The series looks pretty good to me. It easy to follow and makes
Maybe you should decided either to writing rtnl or RTNL everywhere.
Just nitpicking :)
Yes, I'll fix... :-)
> The code in device.c should see a make-over, it's a bit
> is right now. It would be especially useful to colled all interface
> index to interface name mapping into device.c to get rid of an
> ioctl where ConnMan code currently maps index numbers to interface
> name strings and/or back. This requires all plugins to submit
> such information and the best approach here probably is to create a
> new function that creates and registers a device into ConnMan's
> structures. But that's another patch set.
Yep, that is a good plan. Maybe adding to the TODO if you don't have
time to work on it right now?
Let's see. I have at least one low hanging fruit done, but it might be
better to update the existing TODO entry. trigger_rtnl() is now limping
along in patch 3/4, as it does not know the interface name, so that is
to be fixed.
I have a person/project that should verify this all works properly, but
more blacklist testing both ways would be nice...