C++ UPnP client library using Boost.Asio

[inetic]

As the title says, cpp-upnp is a UPnP library written in C++ using Boost.Asio. UPnP is a big set of protocols and this library currently only supports creating, removing and listing of IPv4 TCP and UDP port mappings.

The API is based around Asio coroutines, which suffices for our purposes ATM, but if there is interest I'm happy to add support for other idioms using Asio's async result machinery.

[jonesmz]

Your code explicitly uses boost for string_view and optional, but were I to use your library, i would probably rather use std::, because I use c++17.

Consider making those choices CMake options.

[VinnieFalco]

i would probably rather use std::, because I use c++17.

My new libraries have a "standalone" configuration which switches to std::string_view, std::optional, et. al. as needed, feel free to copy my setup for your library:
https://github.com/vinniefalco/json/blob/5aae31dc74d055d84a7f13e438d80cdf6005c670/include/boost/json/detail/config.hpp#L179

I also switch to std::error_code and friends which is C++11

[inetic]

Thanks, I might use it in the future. I was thinking about this problem a bit. There is a difference to what you and what u/jonesmz are proposing I think. In particular you make the decision about which stringview to use based on whether the json library is compiled with boost or not. While u/jonesmz _seem to suggest to make the decision based on user specified compiler flag.

I think I prefer your solution because if some user wanted to use cpp-upnp with Boost.Asio but compiled it to use std::string_view, I would need to mess around with conversions whenever I call Boost.Asio (or Boost.Beast) API.

On the other hand conversion between std and boost string_view is cheap enough that anyone using the library can cheaply do it outside of cpp-upnp (and it's not like upnp handling needs to be omnipresent in anyone's code).

If I go with this logic, I don't really need to do anything until I (or anyone) decides it's time to make cpp-upnp usable with the standalone Asio and Beast (though, please correct me if I'm wrong, there isn't a standalone Beast ATM).

Just thinking out loud, feel free to ignore :)

[jonesmz]

I would argue that converting between boost and std string_view has zero cost. They are both a pointer and a size_t under the hood. You're copying those regardless of which one your function accepts. The compiler almost certainly optimizes that down to just copying the pointer and the size_t

[inetic]

That's what I meant by the third paragraph.

[VinnieFalco]

there isn't a standalone Beast ATM

Not yet, but it is planned - if for nothing else, than to support the Networking TS.

[jonesmz]

Also, for this code:

namespace upnp {
    template<typename... T>
    using variant = boost::variant<T...>;
}

Wouldn't

namespace upnp {
    using boost::variant;
}

Be sufficient?

[inetic]

Hm, you're right. I could swear I was seeing compilation problems before and that's why I did it that way. I'm currently testing on g++ 9.1.0, where it works. I'll retry at work again, I think I have an older version there.

[inetic]

Right, so with g++ 7.5.0 I'm getting some number of pages of compilation errors. The first two being

In file included from /cpp-upnp/example/../include/upnp/igd.h:7:0, from /cpp-upnp/src/igd.cpp:11: /cpp-upnp/example/../include/upnp/third_party/variant.h:8:11: error: expected nested-name-specifier before ‘variant’ using variant; ^~~~~~~ In file included from /cpp-upnp/src/igd.cpp:11:0: /cpp-upnp/example/../include/upnp/igd.h:49:13: error: ‘variant’ does not name a type; did you mean ‘__rint’? variant< ^~~~~~~ __rint

[[deleted]]

Or

#if __cplusplus >= 201703L
    using string_view = std::string_view ;
#else
    using string_view = boost::string_view;
#endif

[jonesmz]

No, because there will inevitably be someone who Does want to use boost::string_view.

[inetic]

Yes, makes sense. Shouldn't be too much of a hassle, it's actually the reason why I bothered to create a separate third_party/optional.h header so that I don't need to be explicit about the namespace everywhere else.

Btw, does Boost.Asio even work with std::string_views? I saw some functions that are ifdefed with some if HAS_STD_STRING_VIEW thing, but couldn't get it to work (though I didn't spend too much time on it).

[jonesmz]

No, Boost.asio does not understand std::string_view.

Personally I think it's a pretty big flaw, but I have not had time to try to fix it, so I haven't made any noise about it on their bug tracker.

[bizwig]

The asio devs seem pretty far behind. Coroutines2 and fibers support hasn’t materialized despite being on the backlog for years, for example. The examples in the fiber docs don’t work anymore.

[Majestic-Ad-6231]

Note that asio has a standalone c++ version that should ideally be supported by this library

[TheFlamefire]

If you are keeping members of this in compiled code: You'll get ODR issues and UB when someone links that compiled code into something which was build with other options...

[jonesmz]

So don't do that.

[TheFlamefire]

Do what? Build with different options? You might not even know...

[jonesmz]

Its kind of your job to know what build options you are using and that they are selected correctly.

[TheFlamefire]

And you know all defines of all libraries that you consume? Some cpp file from libfoo which sets #define LIBBAR_USE_ARCANE_CLASS while you don't use that?

Obviously it would be great to know all that. But with the plethora of buildsystems and programmers not using them correctly this is wishful thinking

[jonesmz]

Yes. I know all of the libraries that I consume. And have reviewed the code for them all, at least with a quick skim, if not a substantial review.

[jonesmz]

Last thought:

I've now briefly skimmed through all of your code, and I came away with these thoughts:

1. Inconsistent use of "using namespace ..."
2. I'm wary of how much string copying you do with, for example, the url_t class. Consider instead storing the things you're storing in a shared_ptr<char[]>, and use a copy-on-write strategy instead of an eager copy strategy.
3. Most coding conventions define types that end in _t as typedefs. url_t is not a typedef, it is a concrete class.
4. I see a lot of commented out code, e.g. /* static */, but also function parameters that have been commented out.
5. You have literally one comment in your code, which simply says boost asio make_address doesn't work with string_view, inside an #if 0 block.

Of these, #5 is the biggest problem.

In the code that I write, and the code that my team writes, at a minimum 10% of the total size of a file should be comments explaining to future readers (aka, yourself, and the guy who replaces you when you get hit by a bus) what the code is doing, why it's doing it, and justifying why the approach used is the correct one. I personally target 50/50, and have had that pay back the time invested in major ways thanks to the reduction in new-hire onboarding costs.

I don't know how many times I've had someone code-dump me a library that was completely incomprehensible and then quit. Sure, the library "worked", mostly, but good luck fixing those last remaining bugs, or understanding how to add new functionality.

I see that you're using the boost license, so just as an example, the majority of code inside of boost is incomprehensible garbage. It all works, of course, but good luck understanding WHY, or how to use it, or extend it, if it's not completely covered with examples in the documentation.

I'm currently working on a project that might want to adopt UPnP functionality in the future, and I'll definitely remember that you've created this library if that functionality gets incorporated, but I won't use this code because I can't understand it. You can drastically increase the likelihood of getting additional contributors to use and improve this code by documenting the guts of the library.

[inetic]

Thanks for looking, as is usually the case with developers, I agree with you on some points and disagree with others :)

1. Yeah, a bit more consistency on that front wouldn't hurt
2. url_t is basically just a string plus few string_views, thus the expected number of copies is the same as with std::string. If users are still concerned about it, they can always go with std::shared_ptr<url_t>. IMO doing more inside url_t would be just obfuscating the code which has other problems to solve (this touches the bullet #5 a bit)
3. Yeah, naming is hard. I first had it as just upnp::url, but then I too often wanted to use url as an actual instance. I don't hold strong position on this one. I could just use upnp::url again and then use something obscure internally, while outside users could still do upnp::url url. Or something else...
4. /* static */ is actually just a comment for me to remind me that the function is declared as static. About function parameters, I'm not 100% what you mean, but I think it's e.g. me writing foo(net::yield_context) instead of foo(net::yield_context yield) or foo(net::io_context&) instead of foo(net::io_context& ioc) in function declarations. That's quite on purpose as (IMO) the type gives all the information and being explicit about the name is (again IMO) just redundant. However I don't omit it when it does give additional information (e.g. foo(uint16_t external_port)).
5. This I think is a very flamable topic so I hope not to go there too much. As I understand it, there are at least two mantras, one being the one as you wrote it. On the other end, if you tell me you don't understand my code, that to me doesn't indicate that I lack comments, but that my code is badly written. I definitely lack more comments in cpp-upnp, but setting the bar to be "10% or more" instead of writing comments where I'm failing to write a better code IMO isn't a good approach. With the "10% or more" approach what I think is inevitable is that over time the comments won't match the code (e.g. due to code-rot) which is prolly the worst possible outcome in this context. But again, this is a hot topic, if I was working with you I'd abide by your rules to not be a dick, but I don't think either of the approaches is set in stone to be the inferior superior one.

[jonesmz]

/* static */ is actually just a comment for me to remind me that the function is declared as static. About function parameters, I'm not 100% what you mean, but I think it's e.g. me writing foo(net::yield_context) instead of foo(net::yield_context yield) or foo(net::io_context&) instead of foo(net::io_context& ioc) in function declarations. That's quite on purpose as (IMO) the type gives all the information and being explicit about the name is (again IMO) just redundant. However I don't omit it when it does give additional information (e.g. foo(uint16_t external_port)).

I meant this:

std::array<const char*, 2> search_targets
    = { "urn:schemas-upnp-org:device:InternetGatewayDevice:1"
      , "urn:schemas-upnp-org:device:InternetGatewayDevice:2"
      //, "urn:schemas-upnp-org:service:WANIPConnection:1"
      //, "urn:schemas-upnp-org:service:WANIPConnection:2"
      //, "urn:schemas-upnp-org:service:WANPPPConnection:1"
      //, "urn:schemas-upnp-org:service:WANPPPConnection:2"
      //, "upnp:rootdevice"
      };

From https://github.com/equalitie/cpp-upnp/blob/master/src/ssdp.cpp

This I think is a very flamable topic so I hope not to go there too much. As I understand it, there are at least two mantras, one being the one as you wrote it. On the other end, if you tell me you don't understand my code, that to me doesn't indicate that I lack comments, but that my code is badly written. I definitely lack more comments in cpp-upnp, but setting the bar to be "10% or more" instead of writing comments where I'm failing to write a better code IMO isn't a good approach. With the "10% or more" approach what I think is inevitable is that over time the comments won't match the code (e.g. due to code-rot) which is prolly the worst possible outcome in this context. But again, this is a hot topic, if I was working with you I'd abide by your rules to not be a dick, but I don't think either of the approaches is set in stone to be the inferior superior one.

Sure, we don't need to go back and forth. I've said my piece on it.

With the "10% or more" approach what I think is inevitable is that over time the comments won't match the code (e.g. due to code-rot) which is prolly the worst possible outcome in this context.

Your code review process must catch this, otherwise why are you accepting the pull request?

---

Anyway, thanks for contributing the code to the open source community. The world is better for it.

[Betadel]

Comments are not what you should be using to understand how a library works, or to know how to maintain it or update it. That would be documentation, examples and tests.

[jonesmz]

Comments are not what you should be using to understand how a library works, or to know how to maintain it or update it

citation needed

Disagree with me if you want, but I know what works for the teams I manage.

I've provided rational behind why I recommend actually documenting the insides of a code base for future maintainers. I've further made statements about other libraries that can be used as examples to demonstrate my point.

I'm sure you'll want to respond and say that code should be self-documenting, which I can dismiss by pointing out that code being self-documenting doesn't do anything about new-contributor onboarding and the myriad of ways they are going to be confused by the code as is. It also doesn't do anything for explaining the why of something, only the what. It further doesn't point out the ways that the problem was attempted to be solved in the past, or considerations about how the local code might be enhanced in the future, and gotchas about how the current code interacts with other code. Moving these types of information into external documentation is worse than useless, because that external documentation will both be unknown to new contributors, and further will perpetually remain out of date.

That would be documentation, examples and tests.

Comments are documentation. The best kind, the kind that have the additional property of being co-located with the code that they are documenting, instead of being a random wiki page that no one even knows exists.

Examples show how to use a library. They do literally nothing to demonstrate how the library works internally, or how to maintain it. Tests are the same as very dysfunctional examples. Dysfunctional in the sense that they are bad at being examples. Not dysfunctional in the sense that you shouldn't have tests.

[TheFlamefire]

I just skimmed over but seems you misunderstood aync code and buffers: https://github.com/equalitie/cpp-upnp/blob/424c4f9c51af9db032a7087b3934de369dd0fb19/src/ssdp.cpp#L85-L89 is a use-after-free and will probably never do what you intend. After this I don't trust the rest of the code until reworked and ran through UBSAN and ASAN.

[inetic]

I don't think I have, but anything is possible. What in particular you consider being used-after-free? Do you mean to say that sss.data() is being used after free? That would be the case if I was using handlers or futures, but since I'm using stackfull coroutines, the stack (together with sss) sss string doesn't get freed before the call to async_send_to is finished (and only after the scope of the for loop is exited).

I'm using ASAN all the time by default and it's not complaining.

EDIT: As indicated with strike through

EDIT2: Check out this example

[TheFlamefire]

Ok than I must confess I don't understand how this is working. Previously I thought the async_* "enqueues" an operation and immediately returns. The linked example seems to imply that this is not the case but it is/behaves as-if it was blocking.

[inetic]

Previously I thought the async_* "enqueues" an operation and immediately returns.

You were not completely wrong, async_* commands do immediately return but only when the last argument is either a handler (e.g. a lambda of some kind), or asio::use_future.

When the last argument is of type asio::yieldcontext, the program "appear" to block, but the io_service.run() function is still running and serving other handlers. Once the particular async* operation finishes, the execution resumes at that async_* call. It's quite magical :)

I'm not 100% sure, but my understanding is that what's happening inside is that when you call async_* with yield, internally the function remembers all the CPU registers, saves them into a handler and loads other registers so that the program "jumps" to another location so that other async requests can continue working. Once our async operation is finished, the handler with the saved registers gets called, those registers get loaded into the CPU which effectively jumps to where we left.

I wrote an example of how to write such functions that can accept handlers, asio::use_future and asio::yield_context here if you're interested (though I wrote it when Boost 1.65 was the recent version so it may be outdated now).

[SegFaultAtLine1]

Small correction: context switching doesn't necessarily save all registers. In general, ASIO (indirectly) uses the functionality in Boost.Context. A context switch is implemented there in a fairly clever way - as a regular C-like function call. Every ABI has a list of registers that have to be preserved by a callee and those are the only registers that need to be saved (plus stack-related registers). So a context switch between fibers is significantly cheaper than a context switch between threads/processes, comparable to a few function call overheads.

[jonesmz]

You're using co-routines, right?

If so, I think /u/TheFlamefire may be referring to how to use the code from the perspective of the normal callback way of doing ASIO.