search

pupnp / pupnp

libupnp: Build UPnP-compliant control points, devices, and bridges on several operating systems.
https://pupnp.github.io/pupnp
BSD 3-Clause "New" or "Revised" License
352 stars 117 forks source link

Use UpnpInit2() in samples #48

Closed michael-o closed 6 years ago

mrjimenez commented 6 years ago

Hi, Michael,

Would you mind elaborate a bit on your proposal?

Why should we change the initialization on the sample code? I agree we lack a sample with IPV6, so maybe we could create a runtime option to the sample, so that the user could choose if he wants IPV4 or IPV6.

What do you think?

Regards, Marcelo.

michael-o commented 6 years ago

Basically, UpnpInit() is deprecated. Looks like bad habit using deprecated methods in sample code. Moreover, it does not hurt to bind against IPv6 too if a null interface is provided. If no IPv6 is configured, it won't bind anyway. This is just a cleanup.

At best, UpnpInit2() would receive a third argument with an enum value saying IPV4, IPV6 or IPV4_IPV6.

mrjimenez commented 6 years ago

Ok, you are right. Committed.

ukleinek commented 6 years ago

This commit breaks compiling after ./configure --disable-ipv6.

michael-o commented 6 years ago

So we need defined around it and fall back to UpnpInit() if IPv6 has been disabled. Does this make sense?

whyman commented 6 years ago

@michael-o yes, thats the fix.

ukleinek commented 6 years ago

I wonder why there are two init functions. If you ask me the right fix is that libupnp provides UpnpInit2 always. Its a PITA to have different functions provided in a lib depending on configure flags anyhow. Strictly speaking it is wrong to let libupnp with and without UpnpInit2 have the same library version numbers.

whyman commented 6 years ago

@ukleinek I agree, less is more in this case.

In fact, @michael-o has already fixed UpnpInit2() to work without IPv6 being available at runtime, but I am not sure if there are still compile time dependencies.

mrjimenez commented 6 years ago

I have committed a possible fix, please check it. Don't know about possible compile time dependencies, though.