branch th/platform-div-bgo748131

hm... this branch is broken.

I moved the initialization from nm-linux-platform.c:setup() to constructed().
The problem is, that all kind of code calls back into nm_platform functions, which crash due to missing singleton.

+ Trace 234984

A NMPlatform instance is only usable, if the singleton is initialized via nm_platform_setup(). I wish, NMPlatform would be useable without being a singleton. I see usecases for that, for example, we could have a NMPlatform instnce whose netlink socket is connected into another NET namespace.

I would like to modify function in nm-platform.h to accept a @self argument. For convenience, it could accept NULL, which means: use nm_platform_get() instead.


Would that be ok?

-- on hold. First decide hot to proceed with @self argument to platform code.

After merging [1], th/platform-div-bgo748131 is again up for review.

[1] http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=c6529a9d748ad3c8ee37431d020a7b9223992a23

> platform: drop virtual setup() initalization function

I assume the "nm_fake_platform_setup2()" is a typo?

> platform: add _LOG() logging macros to linux platform

What's the point of defining _LOG_LEVEL_ENABLED as quasi-function?  eg, why couldn't it just be "#define _LOG_LEVEL_ENABLED nm_logging_enabled(...)"?

> platform: refactor detection of support_user_ipv6ll

I don't think we need the freeze_result parameter.  The only time 'FALSE' is used is from nm-device.c::constuctor(), and since this call requires a valid ifindex, there's no possible way we can create an NMDevice with a valid ifindex that *doesn't* come from the platform already, right?

Second, before nm_platform_link_get_user_ipv6ll_enabled() gets called, nm-device.c calls nm_platform_check_support_user_ipv6ll(), which itself calls _support_user_ipv6ll_get (TRUE) which freezes the value already.  So I think we can just always freeze the result?

(In reply to Dan Williams from comment #5)
> > platform: drop virtual setup() initalization function
> 
> I assume the "nm_fake_platform_setup2()" is a typo?

reworded commit message.


> > platform: add _LOG() logging macros to linux platform
> 
> What's the point of defining _LOG_LEVEL_ENABLED as quasi-function?  eg, why
> couldn't it just be "#define _LOG_LEVEL_ENABLED nm_logging_enabled(...)"?

fixed, and a few further minor adjustments.


> > platform: refactor detection of support_user_ipv6ll
> 
> I don't think we need the freeze_result parameter.  The only time 'FALSE' is
> used is from nm-device.c::constuctor(), and since this call requires a valid
> ifindex, there's no possible way we can create an NMDevice with a valid
> ifindex that *doesn't* come from the platform already, right?
> 
> Second, before nm_platform_link_get_user_ipv6ll_enabled() gets called,
> nm-device.c calls nm_platform_check_support_user_ipv6ll(), which itself
> calls _support_user_ipv6ll_get (TRUE) which freezes the value already.  So I
> think we can just always freeze the result?

As you say, the situation probably never arises.

And even if it would happen, it would be very strange that support is first FALSE (unknown), and later gets detected. The functions should make up it's mind once and stick with it.

Changed.


Repushed with fixups.

Apart from the nl/ipv6 parts I'm not very familiar with, the rest looks good to me.

>            nm_log ((__level), (__domain), \
>                    "%s: " _NM_UTILS_MACRO_FIRST (__VA_ARGS__), \
>                    __p_prefix _NM_UTILS_MACRO_REST (__VA_ARGS__)); \

Just a minor improvement: __level and __domain aren't macro arguments and so the parenthesis aren't needed.

(In reply to Beniamino Galvani from comment #7)
> Apart from the nl/ipv6 parts I'm not very familiar with, the rest looks good
> to me.
> 
> >            nm_log ((__level), (__domain), \
> >                    "%s: " _NM_UTILS_MACRO_FIRST (__VA_ARGS__), \
> >                    __p_prefix _NM_UTILS_MACRO_REST (__VA_ARGS__)); \
> 
> Just a minor improvement: __level and __domain aren't macro arguments and so
> the parenthesis aren't needed.

fixed.

IMO this branch is trivial enough.

I just went ahead and merged it:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2316d233e36d7c98e4ecad2016ed2467c853a6a0