Skip to content

Correct section name for services in fbtrace config #8500

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 3, 2025
+45 −47
Merged

Correct section name for services in fbtrace config #8500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be846ed
Correct section name for services in fbtrace config
Noremos Apr 2, 2025
645c574
Move section type paring to trace reader class
Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 0 additions & 33 deletions src/common/config/config_file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,36 +960,3 @@ bool ConfigFile::Parameter::asBoolean() const
value.equalsNoCase("y");
}

/******************************************************************************
*
* Parse name as a section key
*/

ConfigFile::SectionType ConfigFile::Parameter::parseSectionKey() const
{
if (name == "database")
{
return SectionType::DATABASE;
}
else if (name == "databaseName")
{
return SectionType::DATABASE_NAME;
}
else if (name == "databaseRegex")
{
return SectionType::DATABASE_REGEX;
}
else if (name == "service")
Copy link
Member

@AlexPeshkoff AlexPeshkoff Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will break existing trace configs. I strongly disagree.

Copy link
Member

@hvlad hvlad Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reviewing #8427 I missed that trace-specific routine was put into common class.
It is ConfigFile::Parameter::parseSectionKey() and related enum class SectionType.
I think both must be moved out of ConfigFile::Parameter into TraceCfgReader.

Copy link
Member

@AlexPeshkoff AlexPeshkoff Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both must be moved out of ConfigFile::Parameter into TraceCfgReader.

+1

Copy link
Contributor Author

@Noremos Noremos Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will break existing trace configs. I strongly disagree.

Existing configurations are using the key 'services'. It was my mistake in the original PR where I incorrectly set the key as 'service'."

Copy link
Contributor Author

@Noremos Noremos Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reviewing #8427 I missed that trace-specific routine was put into common class. It is ConfigFile::Parameter::parseSectionKey() and related enum class SectionType. I think both must be moved out of ConfigFile::Parameter into TraceCfgReader.

I have moved the method

{
return SectionType::SERVICE;
}
else
{
fatal_exception::raiseFmt("error while parsing trace configuration\n\t"
"line %d: wrong section header, \"database\", \"databaseName\", \"databaseRegex\" or \"service\" is expected",
line);

// Return something to calm down the compiler
return SectionType::DATABASE;
}
}
10 changes: 0 additions & 10 deletions src/common/config/config_file.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,6 @@ class ConfigFile : public Firebird::AutoStorage, public Firebird::RefCounted
virtual const char* getFileName() const = 0;
};

// Possible section types
enum class SectionType
{
DATABASE,
DATABASE_NAME,
DATABASE_REGEX,
SERVICE
};

struct Parameter : public AutoStorage
{
Parameter(MemoryPool& p, const Parameter& par)
Expand All @@ -96,7 +87,6 @@ class ConfigFile : public Firebird::AutoStorage, public Firebird::RefCounted

SINT64 asInteger() const;
bool asBoolean() const;
SectionType parseSectionKey() const;

KeyType name;
String value;
Expand Down
39 changes: 35 additions & 4 deletions src/utilities/ntrace/TraceConfiguration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ void TraceCfgReader::readConfig()
{
const ConfigFile::Parameter* section = &params[n];

const ConfigFile::SectionType sectionType = section->parseSectionKey();
const bool isDatabase = (sectionType != ConfigFile::SectionType::SERVICE);
const SectionType sectionType = parseSectionKey(section);
const bool isDatabase = (sectionType != SectionType::SERVICES);

const ConfigFile::String pattern = section->value;
bool match = false;
Expand Down Expand Up @@ -127,14 +127,14 @@ void TraceCfgReader::readConfig()
noQuotePattern.alltrim(" '\'");
PathName expandedName;

if (sectionType != ConfigFile::SectionType::DATABASE_REGEX && (m_databaseName == noQuotePattern ||
if (sectionType != SectionType::DATABASE_REGEX && (m_databaseName == noQuotePattern ||
(expandDatabaseName(noQuotePattern, expandedName, nullptr),
m_databaseName == expandedName) ))
{
// Compare by name
match = exactMatch = true;
}
else if (sectionType != ConfigFile::SectionType::DATABASE_NAME)
else if (sectionType != SectionType::DATABASE_NAME)
{
// Compare by regex
bool regExpOk = false;
Expand Down Expand Up @@ -318,3 +318,34 @@ void TraceCfgReader::expandPattern(const ConfigFile::Parameter* el, PathName& va
pos++;
}
}

TraceCfgReader::SectionType TraceCfgReader::parseSectionKey(const ConfigFile::Parameter* el) const
{
fb_assert(el);

if (el->name == "database")
{
return SectionType::DATABASE;
}
else if (el->name == "databaseName")
{
return SectionType::DATABASE_NAME;
}
else if (el->name == "databaseRegex")
{
return SectionType::DATABASE_REGEX;
}
else if (el->name == "services")
{
return SectionType::SERVICES;
}
else
{
fatal_exception::raiseFmt("error while parsing trace configuration\n\t"
"line %d: wrong section header, \"database\", \"databaseName\", \"databaseRegex\" or \"services\" is expected",
el->line);

// Return something to calm down the compiler
return SectionType::DATABASE;
}
}
10 changes: 10 additions & 0 deletions src/utilities/ntrace/TraceConfiguration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ class TraceCfgReader
int end;
};

// Possible section types
enum class SectionType
{
DATABASE,
DATABASE_NAME,
DATABASE_REGEX,
SERVICES
};

private:
TraceCfgReader(const char* text, const Firebird::PathName& databaseName, TracePluginConfig& config) :
m_text(text),
Expand All @@ -61,6 +70,7 @@ class TraceCfgReader
void expandPattern(const ConfigFile::Parameter* el, Firebird::PathName& valueToExpand);
bool parseBoolean(const ConfigFile::Parameter* el) const;
ULONG parseUInteger(const ConfigFile::Parameter* el) const;
SectionType parseSectionKey(const ConfigFile::Parameter* el) const;

const char* const m_text;
const Firebird::PathName& m_databaseName;
Expand Down
Loading