Clean up after failed initialization.

[thoren-d]

Some initialization failures (such as failure in UpnpGetIfInfo) would leave resources like thread pools initialized. This change ensures if UpnpInit2 fails, we call UpnpFinish before returning to clean up those resources.

To ensure UpnpFinish is effective in this case, we set UpnpSdkInit to 1 near the beginning of UpnpInit2.

[ingo-h]

If I called UpnpInit2() a second time it hasn't modified UpnpSdkInit. It was still 1 as expected. With your changes it is set to 0 what doesn't make sense.

Also if I call UpnpFinish() the first time on an initialized UpnpSdk it returns UPNP_E_FINISH now instead of UPNP_E_SUCCESS.

Description of UPNP_E_FINISH means: "UpnpInit2 has not been called, or UpnpFinish has already been called. None of the API functions operate until UpnpInit2 successfully completes."

Your changes are not compatible.

[thoren-d]

I see the first issue and posted a quick fix.

I'm not sure how you get the second issue, unless UpnpInit2 failed, in which case I would argue it wasn't "initialized" at all.

I could use some guidance on how to fix this bug in an API-compatible way. My best guess is to factor out the cleanup parts of UpnpFinish into a function I can call in case of initialization failure without setting UpnpSdkInit.

[ingo-h]

Hi @thoren-d,

I see the first issue and posted a quick fix.

All issues have gone now.

I'm not sure how you get the second issue, unless UpnpInit2 failed, in which case I would argue it wasn't "initialized" at all.

I use Unit Tests. Earlier setting of UpnpSdkInit may effect functions UpnpInitPreamble() and UpnpGetIfInfo(). If they do not use or modify UpnpSdkInit we can set the flag earlier. This is given. I cannot find side effects to the functions, so I suggest to accept your pull request. But it is not to me to accept it.

I could use some guidance on how to fix this bug in an API-compatible way.

It should be compatible now.

[whyman]

Looks sensible to me

[mrjimenez]

Thank you for reviewing the patch, guys.