Skip to content
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

Allow pcn_log to be split in multiple lines #55

Merged

Conversation

sebymiano
Copy link
Collaborator

@sebymiano sebymiano commented Jan 31, 2019

This commit fixes a "bug" in the pcn_log function that did not allow to split the pcn_log body into multiple lines.
It uses an improved regex function to check the end of the function and replace it with the correct version so that compilation errors can be avoided.

This PR solves the issue #1 reported by @mauriciovasquezbernal.

@frisso
Copy link
Contributor

frisso commented Jan 31, 2019

Not clear to me how it could be used. I suggest to add a brief comment in the documentation saying how to split the content over multiple lines, and also that this practice should be really avoided because it affects the capability to parse the output log with another program (which may expect one log per line).

@mauriciovasquezbernal
Copy link
Contributor

@frisso you're right, the text present on a logging primitive should be in a single line, your reason is a valid one and there is another one, it can be difficult to look for a error string in the code if this is broken into multiple lines.

However this is not the goal of this PR, the goal is to allow splitting the call of a pcn_log in multiple lines, for example putting the text and the parameters in different ones. For example:

now:

pcn_log(ctx, LOG_TRACE, "Received new packet: in_port: %d eth_type: 0x%x mac_src: %M mac_dst: %M", md->in_port, bpf_ntohs(eth->proto), eth->src, eth->dst);

after this PR:

pcn_log(ctx, LOG_TRACE, 
        "Received new packet: in_port: %d eth_type: 0x%x mac_src: %M mac_dst: %M",
        md->in_port, bpf_ntohs(eth->proto), eth->src, eth->dst);

This is weird but the second case is not supported currently as described in #1.

@frisso
Copy link
Contributor

frisso commented Jan 31, 2019

Got it, Mauricio, thank you very much.
I suggest to add this explanation in the commit description; otherwise I feel hard to understand what we're trying to do.

@mauriciovasquezbernal
Copy link
Contributor

I tested it and it works pretty good, there is just one case it doesn't cover (was also not covered on previous version)

pcn_log (....)
       ^
       Space

Would you mind applying the next diff to also supporting it?

Thanks so much for the PR.

diff --git a/Documentation/developers.rst b/Documentation/developers.rst
index 1938a89..c6f071c 100644
--- a/Documentation/developers.rst
+++ b/Documentation/developers.rst
@@ -155,7 +155,6 @@ Two special format specifiers are available:
 Please note the the custom specifiers spec the data to be in network byte order while standard specifiers expects it to be in host by order.
 
 Current limitations:
- - The whole call to `pcn_log` should be on the same line
  - Cannot be used inside a macro
  - Maximum 4 arguments are allowed
 
diff --git a/src/polycubed/src/datapath_log.cpp b/src/polycubed/src/datapath_log.cpp
index 98c04e1..a59b0e5 100644
--- a/src/polycubed/src/datapath_log.cpp
+++ b/src/polycubed/src/datapath_log.cpp
@@ -213,11 +213,11 @@ std::string DatapathLog::replace_string(std::string& subject, const std::string&
 static std::regex regComments(R"***((?:\/\/(?:\\\n|[^\n])*)|(?:\/\*[\s\S]*?\*\/)|((?:R"([^(\\\s]{0,16})\([^)]*\)\2")|(?:@"[^"]*?")|(?:"(?:\?\?'|\\\\|\\"|\\\n|[^"])*?")|(?:'(?:\\\\|\\'|\\\n|[^'])*?')))***");
 
 // matches all calls to pcn_log(ctx, ...)
-static std::regex regN(R"***(pcn_log\(([\s\S]+?)\)\s*;)***");
+static std::regex regN(R"***(pcn_log\s*\(([\s\S]+?)\)\s*;)***");
 static std::regex regNewLine(R"***(/(\r\n)+|\r+|\n+|\t+/i)***");
 static std::regex regAddSpaces(R"***( +)***");
 
-static std::regex regNPkt(R"***(pcn_pkt_log\(([\s\S]+?)\)\s*;)***");
+static std::regex regNPkt(R"***(pcn_pkt_log\s*\(([\s\S]+?)\)\s*;)***");
 
 std::string DatapathLog::dp_callback(const std::smatch& m) {
     std::string match = std::regex_replace(m.str(1), regNewLine, "");

@sebymiano sebymiano force-pushed the pr/fix_dp_log_multiline branch from 482fca5 to 39f86a9 Compare February 1, 2019 10:12
This commit fixes a "bug" in the pcn_log function that did not allow
to split the pcn_log body into multiple lines, for example by putting
the text and the parameters in different lines.
It uses an improved regex function to check the end of the
pcn_log function and replace it with the correct version so that
compilation errors can be avoided.

Fixes: polycube-network#1 (pcn_log cannot be split in multiple lines)
Signed-off-by: Sebastiano Miano <mianosebastiano@gmail.com>
@sebymiano sebymiano force-pushed the pr/fix_dp_log_multiline branch from 39f86a9 to 1fa9d1d Compare February 1, 2019 15:00
@sebymiano
Copy link
Collaborator Author

Thanks for the clarification @mauriciovasquezbernal.
I've applied your patch and slightly modified the commit message to make it more clear.

Copy link
Contributor

@mbertrone mbertrone left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Sebastiano.

@sebymiano sebymiano merged commit 5a8c2c9 into polycube-network:master Feb 1, 2019
@sebymiano sebymiano deleted the pr/fix_dp_log_multiline branch February 1, 2019 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants