-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
[multicapture 1/3] Refactor to extract useful functions/constants for multi-capture #837
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,7 @@ | |
#include "../../server/tracy_robin_hood.h" | ||
#include "../../server/TracyFileHeader.hpp" | ||
#include "../../server/TracyFileRead.hpp" | ||
#include "../../server/TracyProtocolServer.hpp" | ||
#include "../../server/TracyPrint.hpp" | ||
#include "../../server/TracySysUtil.hpp" | ||
#include "../../server/TracyWorker.hpp" | ||
|
@@ -85,7 +86,7 @@ enum class ViewShutdown { False, True, Join }; | |
static tracy::unordered_flat_map<uint64_t, ClientData> clients; | ||
static std::unique_ptr<tracy::View> view; | ||
static tracy::BadVersionState badVer; | ||
static uint16_t port = 8086; | ||
static uint16_t port = tracy::DEFAULT_BROADCAST_UDP_PORT; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the point of making this a value defined elsewhere? (Where? I now have to actively seek for it.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was extracting it because we use it in multicapture as well (3 times IIRC). As this was currently in a .cpp file of I put it in [I can revert this extraction and use the value directly if you prefer - but I was trying to avoid having multiple |
||
static const char* connectTo = nullptr; | ||
static char title[128]; | ||
static std::thread loadThread, updateThread, updateNotesThread; | ||
|
@@ -452,76 +453,15 @@ static void UpdateBroadcastClients() | |
{ | ||
auto msg = broadcastListen->Read( len, addr, 0 ); | ||
if( !msg ) break; | ||
if( len > sizeof( tracy::BroadcastMessage ) ) continue; | ||
uint16_t broadcastVersion; | ||
memcpy( &broadcastVersion, msg, sizeof( uint16_t ) ); | ||
if( broadcastVersion <= tracy::BroadcastVersion ) | ||
auto parsedMessageOpt = tracy::ParseBroadcastMessage(msg, len); | ||
if (parsedMessageOpt.has_value()) | ||
{ | ||
uint32_t protoVer; | ||
char procname[tracy::WelcomeMessageProgramNameSize]; | ||
int32_t activeTime; | ||
uint16_t listenPort; | ||
uint64_t pid; | ||
|
||
switch( broadcastVersion ) | ||
{ | ||
case 3: | ||
{ | ||
tracy::BroadcastMessage bm; | ||
memcpy( &bm, msg, len ); | ||
protoVer = bm.protocolVersion; | ||
strcpy( procname, bm.programName ); | ||
activeTime = bm.activeTime; | ||
listenPort = bm.listenPort; | ||
pid = bm.pid; | ||
break; | ||
} | ||
case 2: | ||
{ | ||
if( len > sizeof( tracy::BroadcastMessage_v2 ) ) continue; | ||
tracy::BroadcastMessage_v2 bm; | ||
memcpy( &bm, msg, len ); | ||
protoVer = bm.protocolVersion; | ||
strcpy( procname, bm.programName ); | ||
activeTime = bm.activeTime; | ||
listenPort = bm.listenPort; | ||
pid = 0; | ||
break; | ||
} | ||
case 1: | ||
{ | ||
if( len > sizeof( tracy::BroadcastMessage_v1 ) ) continue; | ||
tracy::BroadcastMessage_v1 bm; | ||
memcpy( &bm, msg, len ); | ||
protoVer = bm.protocolVersion; | ||
strcpy( procname, bm.programName ); | ||
activeTime = bm.activeTime; | ||
listenPort = bm.listenPort; | ||
pid = 0; | ||
break; | ||
} | ||
case 0: | ||
{ | ||
if( len > sizeof( tracy::BroadcastMessage_v0 ) ) continue; | ||
tracy::BroadcastMessage_v0 bm; | ||
memcpy( &bm, msg, len ); | ||
protoVer = bm.protocolVersion; | ||
strcpy( procname, bm.programName ); | ||
activeTime = bm.activeTime; | ||
listenPort = 8086; | ||
pid = 0; | ||
break; | ||
} | ||
default: | ||
assert( false ); | ||
break; | ||
} | ||
|
||
auto parsedMessage = parsedMessageOpt.value(); | ||
auto address = addr.GetText(); | ||
const auto clientId = tracy::ClientUniqueID(addr, parsedMessage.listenPort); | ||
const auto ipNumerical = addr.GetNumber(); | ||
const auto clientId = uint64_t( ipNumerical ) | ( uint64_t( listenPort ) << 32 ); | ||
auto it = clients.find( clientId ); | ||
if( activeTime >= 0 ) | ||
if( parsedMessage.activeTime >= 0 ) | ||
{ | ||
if( it == clients.end() ) | ||
{ | ||
|
@@ -538,16 +478,24 @@ static void UpdateBroadcastClients() | |
} ); | ||
} | ||
resolvLock.unlock(); | ||
clients.emplace( clientId, ClientData { time, protoVer, activeTime, listenPort, pid, procname, std::move( ip ) } ); | ||
clients.emplace(clientId, | ||
ClientData{time, | ||
parsedMessage.protocolVersion, | ||
parsedMessage.activeTime, | ||
parsedMessage.listenPort, | ||
parsedMessage.pid, | ||
parsedMessage.programName, | ||
std::move(ip)}); | ||
} | ||
else | ||
{ | ||
it->second.time = time; | ||
it->second.activeTime = activeTime; | ||
it->second.port = listenPort; | ||
it->second.pid = pid; | ||
it->second.protocolVersion = protoVer; | ||
if( strcmp( it->second.procName.c_str(), procname ) != 0 ) it->second.procName = procname; | ||
it->second.activeTime = parsedMessage.activeTime; | ||
it->second.port = parsedMessage.listenPort; | ||
it->second.pid = parsedMessage.pid; | ||
it->second.protocolVersion = parsedMessage.protocolVersion; | ||
if (strcmp(it->second.procName.c_str(), parsedMessage.programName) != 0) | ||
it->second.procName = parsedMessage.programName; | ||
} | ||
} | ||
else if( it != clients.end() ) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -651,7 +651,7 @@ void IpAddress::Set( const struct sockaddr& addr ) | |
#else | ||
auto ai = (const struct sockaddr_in*)&addr; | ||
#endif | ||
inet_ntop( AF_INET, &ai->sin_addr, m_text, 17 ); | ||
inet_ntop(AF_INET, &ai->sin_addr, m_text, TEXT_SIZE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this changed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this constant is being used in multicapture 3 times. I thought best to extract it to give it a name. I agree TEXT_SIZE is not a good name; maybe you have a suggestion [as other extraction changes in this MR, I can revert it - but I'll have to copy-paste its value where it's used, and that felt less clean and understandable when diving into the codebase] |
||
m_number = ai->sin_addr.s_addr; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
#include "TracyProtocolServer.hpp" | ||
#include <cassert> | ||
#include <cstring> | ||
#include <iostream> | ||
|
||
namespace tracy | ||
{ | ||
std::optional<tracy::BroadcastMessage> ParseBroadcastMessage(const char* msg, size_t msgLen) | ||
{ | ||
if (msgLen < sizeof(uint16_t)) | ||
{ | ||
std::cout << "Received too short broadcast message" << std::endl; | ||
return std::nullopt; | ||
} | ||
uint16_t broadcastVersion; | ||
memcpy(&broadcastVersion, msg, sizeof(uint16_t)); | ||
if (broadcastVersion > tracy::BroadcastVersion) | ||
{ | ||
std::cout << "Received broadcast message with unsupported version: " << broadcastVersion << std::endl; | ||
return std::nullopt; | ||
} | ||
switch (broadcastVersion) | ||
{ | ||
case 3: | ||
{ | ||
if (msgLen > sizeof(tracy::BroadcastMessage)) | ||
{ | ||
std::cout << "Received unexpected size broadcast v3 message" << std::endl; | ||
return std::nullopt; | ||
} | ||
tracy::BroadcastMessage bm; | ||
memcpy(&bm, msg, msgLen); | ||
return bm; | ||
break; | ||
} | ||
case 2: | ||
{ | ||
if (msgLen > sizeof(tracy::BroadcastMessage_v2)) | ||
{ | ||
std::cout << "Received unexpected size broadcast v2 message" << std::endl; | ||
return std::nullopt; | ||
} | ||
tracy::BroadcastMessage_v2 bm; | ||
memcpy(&bm, msg, msgLen); | ||
|
||
tracy::BroadcastMessage out; | ||
out.broadcastVersion = broadcastVersion; | ||
out.protocolVersion = bm.protocolVersion; | ||
out.activeTime = bm.activeTime; | ||
out.listenPort = bm.listenPort; | ||
strcpy(out.programName, bm.programName); | ||
out.pid = 0; | ||
return out; | ||
break; | ||
} | ||
case 1: | ||
{ | ||
if (msgLen > sizeof(tracy::BroadcastMessage_v1)) | ||
{ | ||
std::cout << "Received unexpected size broadcast v1 message" << std::endl; | ||
return std::nullopt; | ||
} | ||
tracy::BroadcastMessage_v1 bm; | ||
memcpy(&bm, msg, msgLen); | ||
|
||
tracy::BroadcastMessage out; | ||
out.broadcastVersion = broadcastVersion; | ||
out.protocolVersion = bm.protocolVersion; | ||
out.activeTime = bm.activeTime; | ||
out.listenPort = bm.listenPort; | ||
strcpy(out.programName, bm.programName); | ||
out.pid = 0; | ||
return out; | ||
break; | ||
} | ||
case 0: | ||
{ | ||
if (msgLen > sizeof(tracy::BroadcastMessage_v0)) | ||
{ | ||
std::cout << "Received unexpected size broadcast v0 message" << std::endl; | ||
return std::nullopt; | ||
} | ||
tracy::BroadcastMessage_v0 bm; | ||
memcpy(&bm, msg, msgLen); | ||
|
||
tracy::BroadcastMessage out; | ||
out.broadcastVersion = broadcastVersion; | ||
out.protocolVersion = bm.protocolVersion; | ||
out.activeTime = bm.activeTime; | ||
out.listenPort = tracy::DEFAULT_CLIENT_DATA_TCP_PORT; | ||
strcpy(out.programName, bm.programName); | ||
out.pid = 0; | ||
return out; | ||
break; | ||
} | ||
default: | ||
assert(false); | ||
break; | ||
} | ||
return std::nullopt; | ||
} | ||
|
||
uint64_t ClientUniqueID(tracy::IpAddress const& addr, uint16_t port) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the id "unique"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume the client cannot connect with different IP addresses and ports along its lifetime, so this info can uniquely identify him ? Multicapture uses this as a key to the map of clients, so knowing if it's unique was important. that said, |
||
{ | ||
return uint64_t(addr.GetNumber()) | (uint64_t(port) << 32); | ||
} | ||
} // namespace tracy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// server-side functions supporting the protocol | ||
#ifndef __TRACYPROTOCOLSERVER_HPP__ | ||
#define __TRACYPROTOCOLSERVER_HPP__ | ||
|
||
#include <optional> | ||
#include "TracyProtocol.hpp" | ||
#include "TracySocket.hpp" | ||
|
||
namespace tracy | ||
{ | ||
// create the latest version of broadcast message, migrating older versions if possible | ||
std::optional<tracy::BroadcastMessage> ParseBroadcastMessage(const char* msg, size_t msgLen); | ||
// internal unique ID for a client | ||
uint64_t ClientUniqueID(tracy::IpAddress const& addr, uint16_t port); | ||
} // namespace tracy | ||
|
||
#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.
What is the purpose of this change? How is writing or reading this much text more clear in intent than a negative value?
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.
the point was less to clarify the constant than being able to see where this logic is used in other files. There's a lot
-1
constants in the codebase, but renaming it enabled to find all the places we were talking about memory limits.This is not adding any feature, and you're the maintainer here, so if you prefer
tracy::NO_LIMIT
or even me keeping several-1
around I can do that. (remark applies to most of the proposed changes here - their goal was to limit code duplication where I understood it well enough to operate changes)