Add support for SRTO_STREAMID #34
Conversation
add SRTO_STREAMID in NodeSRT::GetSockOpt
added String as possible Value to setSockOpt added test for normal SRT API removed double SRTO_STREAMID from srt-enums.h
|
Hi @Morpheus235 :) Only got to look closely at the current master here just now and the diff to our fork (this patch effectively). Unfortunately there is indeed an issue with that (see my comments later). As you mentioned, C/C++ can be a tricky terrain, even for the experienced :) Thanks so much either way for the contribution, stick to it pls, and to C (even w/o the ++ ;p). Feel free to ask any Qs regarding the specific points here. @birme Have already crafted a patch here that gets this all round: #37 Please review, thanks! |
| Napi::String value = info[2].As<Napi::String>(); | ||
| int32_t optName = option; | ||
| const char * optValue = std::string(value).c_str(); | ||
| result = srt_setsockflag(socketValue, (SRT_SOCKOPT)optName, optValue, sizeof(string)); |
There was a problem hiding this comment.
sizeof(string) is an relatively arbitrary and likely platform C++ runtime lib dependent allocation of the std::string container size, as in how much stack/heap gets allocated for that object as in per instance :D that is a compile-time constant also obviously. It is not related the string length, because that class is just a container for the underlying C-string data buffer.
The srt_setsockflag expects the size of the data behind the pointer passed. that is constant for bools and ints ie numbers. In case of a string, the length of the string is expected.
There was a problem hiding this comment.
Just adding here also, this does "somehow" work as long as your string data passed is smaller than whatever sizeof(string) gives you. But that shouldnt be the idea here ;)
probably somewhat contributing to it having worked at all is that C-string is null-terminated, so even if libSRT copies this into a too large buffer along with some garbage, it reads the string properly ... but still this could crash as potentially letting srt_setsockflag read/copy from random place in memory itfp -- just in case this ever crashed for you; that is probably why.
| } else if (info[2].IsString()) { | ||
| Napi::String value = info[2].As<Napi::String>(); | ||
| int32_t optName = option; | ||
| const char * optValue = std::string(value).c_str(); |
There was a problem hiding this comment.
Clang compiler spits out a warning here for understandable reasons.
that std::string objects may be kicked off the stack just after c_str returns.
meaning that the mem at the pointer is freed then, and potentially the value passed not ensured to be what you expect here at all (even if this may work, you actually pass a pointer to already deallocated mem here every time).
I added the missing Pieces for getting SRTO_STREAMID to work by adding String as supported Value to setSockOpt and adding it in the getSockOpt switch case. I also added Testcases for both APIs. Feel free to correct anything wrong, as I am pretty unexperienced when it comes to C++ code.