-
Notifications
You must be signed in to change notification settings - Fork 849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] Fixed the use of the right error constant #2595
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2595 +/- ##
==========================================
+ Coverage 64.26% 64.40% +0.13%
==========================================
Files 101 101
Lines 17480 17512 +32
==========================================
+ Hits 11233 11278 +45
+ Misses 6247 6234 -13 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more for 1.6.0 due to changes to the API.
|
||
#include <ostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright header is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIght. Even though this file is normally unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference with utilities.h
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file contains things that are in any temporary use for development. This is only provided for convenience for the use during development, but the normal code goes with this file ousted, with provided just plain definitions for the specific types as alias to integer.
Directly this file is provided so that by a temporary change in srt.h you can run the compile command and then by the errors you get you can find out where the rules for the right usage of the status types has been violated.
srtcore/srt.h
Outdated
#ifdef __cplusplus | ||
namespace srt { | ||
inline bool isgroup(SRTSOCKET sid) { return (int32_t(sid) & SRTGROUP_MASK) != 0; } | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ API (inline function) mixed with C API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be moved. I wanted to make it available for the users. We can just as well add this to the internals and later move it to the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer not to change the srt.h
in the 1.5.4 patch release.
…ssing license heading.
Fixes #2505
This changes the use of the error constants SRT_ERROR and SRT_INVALID_SOCK. Besides, there are some others (maybe some better fix could be introduced sometimes, but note that some parts apply to the C API):
SRTSTATUS
. This is obviously an alias forint
, but the use of it suggests that there are only two possible values expected in return:SRT_ERROR
orSRT_STATUS_OK
(newly introduced in this PR - could beSRT_SUCCESS
, but this symbol is already used for a 0 error code, so it's from a kinda different domain).SRTRUNSTATUS
, which contains values for usual OK and ERROR, but also ALREADY, which means that no actual action was undertaken as it was started already. This is currently in use forsrt_startup()
, as it was hard to find any better solution (an integer value could be used, but this value is still symbolic, not anyhow numeric with trap), but made universal enough if any need for similar type of status appears.SRT_ERROR
should be used. This is used explicitly now, although the return type isint
, notSRTSTATUS
.SRT_INVALID_SOCK
as per theSRTSOCKET
return type. As this is also an alias toint
(int32_t
more preciesly), then there's nothing wrong with comparing it toSRT_ERROR
, but the internal code is consequently usingSRT_INVALID_SOCK
whereverSRTSOCKET
type is used.srt_connect
, returnSRTSOCKET
even though the usual use ofsrt_connect
is connecting a single socket and the returned value should be treated as a status. Therefore the pair ofSRT_ERROR
andSRT_STATUS_OK
should be treated as equivalent toSRT_INVALID_SOCK
andSRT_SOCKID_CONNREQ
respectively. The latter is a value of the socket that is used only internally, it's not a valid value of a socket, but also not erroneous (as erroneous is onlySRT_INVALID_SOCK
).Might be then that it deserves a description, which should mention something like this:
SRTSTATUS
type contains only two possible values:SRT_ERROR
andSRT_STATUS_OK
. When this is the return type in an API function, all values except these are illegal.int
type is an extended version of SRTSTATUS with added positive meaningful values. In this case you are allowed to useSRT_ERROR
and non-negative values in comparison, others are illegal.SRTSOCKET
is also an extended version ofSRTSTATUS
, in whichSRT_INVALID_SOCK
andSRT_SOCKID_CONNREQ
map toSRT_ERROR
andSRT_STATUS_OK
respectively. This way,SRT_SOCKID_CONNREQ
should be treated as a possible return for a function, which in particular runtime circumstances is not expected to return a valid socket ID. This is the case ofsrt_connect
, when it is called for a single socket.TODO:
srt_accept
andsrt_connect
and similar.