[jira] [Commented] (DISPATCH-1568) add c-unittest support
[
https://issues.apache.org/jira/browse/DISPATCH-1568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17039617#comment-17039617
]
ASF GitHub Bot commented on DISPATCH-1568:
--
nicob87 commented on pull request #684: WIP: (Work In Progress) DISPATCH-1568:
using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381033409
##
File path: src/router_core/terminus.c
##
@@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term)
free_qdr_terminus_t(term);
}
+#ifdef TESTING
+__attribute__((weak))
+int mocked_vsnprintf(char *str, size_t size, const char *format, ...){return
-1;}
+#endif
// DISPATCH-1461: snprintf() is evil - it returns >= size on overflow. This
// wrapper will never return >= size, even if truncated. This makes it safe to
// do pointer & length arithmetic without overflowing the destination buffer in
// qdr_terminus_format()
-//
-static inline int safe_snprintf(char *str, size_t size, const char *format,
...)
-{
+// not static to be used unit-tested
+size_t safe_snprintf(char *str, size_t size, const char *format, ...) {
+// max size allowed must be INT_MAX (since vsnprintf reutrns an int)
+if (size == 0 || size > INT_MAX) {
+//TODO log a warning somewhere?
+return 0;
+}
+int max_possible_return_value = (int)(size - 1);
va_list ap;
va_start(ap, format);
+#ifdef TESTING
+int rc = mocked_vsnprintf(str, size, format, ap);
Review comment:
Hi @jdanekrh, Awesome review! Thanks.
About the preprocessor, yes, I agree that code in this pr is polluted and it
is possible to do it cleaner, with a preprocessor "seam". More over that I do
not like using preprocessor at all. And also I do not like to compile twice
just to be able to use the preprocessor. BUT I was looking for a way to do not
pollute the production "binary". I didn't know a couple of tricks explained in
your documentation.
I think it is possible to do something like what is explained in [2]
(Run-time function interception) with the addition here we are mixing c and
c++. It looks like it may work :-P.
Will give it a try tomorrow or one of this days.
Regards!
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> add c-unittest support
> --
>
> Key: DISPATCH-1568
> URL: https://issues.apache.org/jira/browse/DISPATCH-1568
> Project: Qpid Dispatch
> Issue Type: Improvement
> Components: Router Node
>Reporter: Nicolas
>Priority: Major
> Original Estimate: 336h
> Remaining Estimate: 336h
>
> Right now we are not using any framework for easily write c unit-test when
> developing (or later).
> The idea is to research available libraries, pick one and try to include in
> our workflow.
> preliminary candidates:
> [https://github.com/google/googletest]
> https://github.com/catchorg/Catch2
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
nicob87 commented on a change in pull request #684: WIP: (Work In Progress)
DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381033409
##
File path: src/router_core/terminus.c
##
@@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term)
free_qdr_terminus_t(term);
}
+#ifdef TESTING
+__attribute__((weak))
+int mocked_vsnprintf(char *str, size_t size, const char *format, ...){return
-1;}
+#endif
// DISPATCH-1461: snprintf() is evil - it returns >= size on overflow. This
// wrapper will never return >= size, even if truncated. This makes it safe to
// do pointer & length arithmetic without overflowing the destination buffer in
// qdr_terminus_format()
-//
-static inline int safe_snprintf(char *str, size_t size, const char *format,
...)
-{
+// not static to be used unit-tested
+size_t safe_snprintf(char *str, size_t size, const char *format, ...) {
+// max size allowed must be INT_MAX (since vsnprintf reutrns an int)
+if (size == 0 || size > INT_MAX) {
+//TODO log a warning somewhere?
+return 0;
+}
+int max_possible_return_value = (int)(size - 1);
va_list ap;
va_start(ap, format);
+#ifdef TESTING
+int rc = mocked_vsnprintf(str, size, format, ap);
Review comment:
Hi @jdanekrh, Awesome review! Thanks.
About the preprocessor, yes, I agree that code in this pr is polluted and it
is possible to do it cleaner, with a preprocessor "seam". More over that I do
not like using preprocessor at all. And also I do not like to compile twice
just to be able to use the preprocessor. BUT I was looking for a way to do not
pollute the production "binary". I didn't know a couple of tricks explained in
your documentation.
I think it is possible to do something like what is explained in [2]
(Run-time function interception) with the addition here we are mixing c and
c++. It looks like it may work :-P.
Will give it a try tomorrow or one of this days.
Regards!
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (DISPATCH-1568) add c-unittest support
[ https://issues.apache.org/jira/browse/DISPATCH-1568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17039594#comment-17039594 ] ASF GitHub Bot commented on DISPATCH-1568: -- nicob87 commented on pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381019059 ## File path: src/router_core/terminus.c ## @@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term) free_qdr_terminus_t(term); } +#ifdef TESTING +__attribute__((weak)) Review comment: no need to make it weak since this goes to a shared library. __attribute__((noinline)) probably is a good idea This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > add c-unittest support > -- > > Key: DISPATCH-1568 > URL: https://issues.apache.org/jira/browse/DISPATCH-1568 > Project: Qpid Dispatch > Issue Type: Improvement > Components: Router Node >Reporter: Nicolas >Priority: Major > Original Estimate: 336h > Remaining Estimate: 336h > > Right now we are not using any framework for easily write c unit-test when > developing (or later). > The idea is to research available libraries, pick one and try to include in > our workflow. > preliminary candidates: > [https://github.com/google/googletest] > https://github.com/catchorg/Catch2 > -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381019059 ## File path: src/router_core/terminus.c ## @@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term) free_qdr_terminus_t(term); } +#ifdef TESTING +__attribute__((weak)) Review comment: no need to make it weak since this goes to a shared library. __attribute__((noinline)) probably is a good idea This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (DISPATCH-1568) add c-unittest support
[ https://issues.apache.org/jira/browse/DISPATCH-1568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17039593#comment-17039593 ] ASF GitHub Bot commented on DISPATCH-1568: -- nicob87 commented on pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381018665 ## File path: tests/test_terminus_mocks.c ## @@ -0,0 +1,32 @@ +#include +#include +#include +#include +#include +#include +#include Review comment: only string.h, stdbool.h and stdarg.h are needed here This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > add c-unittest support > -- > > Key: DISPATCH-1568 > URL: https://issues.apache.org/jira/browse/DISPATCH-1568 > Project: Qpid Dispatch > Issue Type: Improvement > Components: Router Node >Reporter: Nicolas >Priority: Major > Original Estimate: 336h > Remaining Estimate: 336h > > Right now we are not using any framework for easily write c unit-test when > developing (or later). > The idea is to research available libraries, pick one and try to include in > our workflow. > preliminary candidates: > [https://github.com/google/googletest] > https://github.com/catchorg/Catch2 > -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381018665 ## File path: tests/test_terminus_mocks.c ## @@ -0,0 +1,32 @@ +#include +#include +#include +#include +#include +#include +#include Review comment: only string.h, stdbool.h and stdarg.h are needed here This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (DISPATCH-1575) Core thread logs to ROUTER module instead of ROUTER_CORE module
[ https://issues.apache.org/jira/browse/DISPATCH-1575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17039470#comment-17039470 ] ASF GitHub Bot commented on DISPATCH-1575: -- ganeshmurthy commented on pull request #687: DISPATCH-1575: Made core log to ROUTER_CORE module URL: https://github.com/apache/qpid-dispatch/pull/687 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Core thread logs to ROUTER module instead of ROUTER_CORE module > --- > > Key: DISPATCH-1575 > URL: https://issues.apache.org/jira/browse/DISPATCH-1575 > Project: Qpid Dispatch > Issue Type: Improvement > Components: Container >Affects Versions: 1.10.0 >Reporter: Ganesh Murthy >Assignee: Ganesh Murthy >Priority: Major > Fix For: 1.11.0 > > > The schema has a ROUTER_CORE module. The core thread is supposed to log to > this module. It instead logs to the ROUTER module -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy opened a new pull request #687: DISPATCH-1575: Made core log to ROUTER_CORE module
ganeshmurthy opened a new pull request #687: DISPATCH-1575: Made core log to ROUTER_CORE module URL: https://github.com/apache/qpid-dispatch/pull/687 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (DISPATCH-1518) Add ability to turn on router trace logging for a specific connection
[ https://issues.apache.org/jira/browse/DISPATCH-1518?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17039417#comment-17039417 ] ASF subversion and git services commented on DISPATCH-1518: --- Commit bc085461093511d22a5574a5ce54b41177379b13 in qpid-dispatch's branch refs/heads/master from Ganesh Murthy [ https://gitbox.apache.org/repos/asf?p=qpid-dispatch.git;h=bc08546 ] DISPATCH-1518: Additional fix to clean_attrs to clean only string type attributes > Add ability to turn on router trace logging for a specific connection > -- > > Key: DISPATCH-1518 > URL: https://issues.apache.org/jira/browse/DISPATCH-1518 > Project: Qpid Dispatch > Issue Type: Improvement > Components: Container >Affects Versions: 1.9.0 >Reporter: Ganesh Murthy >Assignee: Ganesh Murthy >Priority: Major > Fix For: 1.11.0 > > > It would be nice to be able to turn on tracing per connection, e.g. something > like `qdmanage update --type=connection id=897644 tracing=true` > That way you could benefit from protocol level tracing on connections of > interest as and when needed. Perhaps also there could be some way of > specifying a policy for connection tracing, e.g. all connections whose > container id is 'foo' or who are connecting from 192.666.555.444 or similar. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Created] (DISPATCH-1576) Allow Q3 session limit to be disabled/enabled per link
Ken Giusti created DISPATCH-1576: Summary: Allow Q3 session limit to be disabled/enabled per link Key: DISPATCH-1576 URL: https://issues.apache.org/jira/browse/DISPATCH-1576 Project: Qpid Dispatch Issue Type: Improvement Components: Router Node Affects Versions: 1.10.0 Reporter: Ken Giusti Assignee: Ken Giusti Fix For: 1.11.0 Q3 session limit blocking needs to be disabled on inter-router links. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (DISPATCH-1569) Add total memory usage to router management entity
[ https://issues.apache.org/jira/browse/DISPATCH-1569?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17039225#comment-17039225 ] ASF subversion and git services commented on DISPATCH-1569: --- Commit d12ad85958e78ce76c6e1c6e413ec180b24d747e in qpid-dispatch's branch refs/heads/master from Ken Giusti [ https://gitbox.apache.org/repos/asf?p=qpid-dispatch.git;h=d12ad85 ] DISPATCH-1569: fix sscanf format string > Add total memory usage to router management entity > -- > > Key: DISPATCH-1569 > URL: https://issues.apache.org/jira/browse/DISPATCH-1569 > Project: Qpid Dispatch > Issue Type: New Feature > Components: Management Agent, Router Node, Tools >Affects Versions: 1.10.0 >Reporter: Ken Giusti >Assignee: Ken Giusti >Priority: Minor > Fix For: 1.11.0 > > > It would be useful to be able to access the current overall memory usage of > the qdrouterd process via management. > This proposal would include > * add a in-use memory gauge to the org.apache.qpid.dispatch.router entity > * add a max available memory gauge to the same entity (optional) > * integrate these values into the qdstat -m output > These gauges should report back "0" if the platform does not provide the > ability to access these metrics in process. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (QPIDJMS-488) update to`Netty 4.1.45
[ https://issues.apache.org/jira/browse/QPIDJMS-488?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17039115#comment-17039115 ] ASF subversion and git services commented on QPIDJMS-488: - Commit e6fa1e37d625986e11112e727a47b9ff3a8d in qpid-jmsgs branch refs/heads/master from Robbie Gemmell [ https://gitbox.apache.org/repos/asf?p=qpid-jms.git;h=e6fa1e3 ] QPIDJMS-488: also update test dep netty-tcnative-boringssl-static to 2.0.29 > update to Netty 4.1.45 > -- > > Key: QPIDJMS-488 > URL: https://issues.apache.org/jira/browse/QPIDJMS-488 > Project: Qpid JMS > Issue Type: Task > Components: qpid-jms-client >Reporter: Robbie Gemmell >Assignee: Robbie Gemmell >Prio2ity: Major > Fix For: 0.49.0 > > > update to Netty 4.1.45 -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (DISPATCH-1568) add c-unittest support
[
https://issues.apache.org/jira/browse/DISPATCH-1568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17039010#comment-17039010
]
ASF GitHub Bot commented on DISPATCH-1568:
--
jdanekrh commented on pull request #684: WIP: (Work In Progress) DISPATCH-1568:
using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r380620641
##
File path: src/router_core/terminus.c
##
@@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term)
free_qdr_terminus_t(term);
}
+#ifdef TESTING
+__attribute__((weak))
+int mocked_vsnprintf(char *str, size_t size, const char *format, ...){return
-1;}
+#endif
// DISPATCH-1461: snprintf() is evil - it returns >= size on overflow. This
// wrapper will never return >= size, even if truncated. This makes it safe to
// do pointer & length arithmetic without overflowing the destination buffer in
// qdr_terminus_format()
-//
-static inline int safe_snprintf(char *str, size_t size, const char *format,
...)
-{
+// not static to be used unit-tested
+size_t safe_snprintf(char *str, size_t size, const char *format, ...) {
+// max size allowed must be INT_MAX (since vsnprintf reutrns an int)
+if (size == 0 || size > INT_MAX) {
+//TODO log a warning somewhere?
+return 0;
+}
+int max_possible_return_value = (int)(size - 1);
va_list ap;
va_start(ap, format);
+#ifdef TESTING
+int rc = mocked_vsnprintf(str, size, format, ap);
Review comment:
I was thinking about this and I don't like this much. I understand that in
C, there isn't many options to replace vsnprintf calls in tests, but this
"pollutes" the production code a lot.
In the C++ world, they seem to call these techniques "seams", [1] [2] [3].
Dispatch already uses the linker seam in two places, to mock
`qd_server_timeout` and `qd_timer_now`.
I generally like using the linker better, it seems cleaner to me.
[1] http://www.informit.com/articles/article.aspx?p=359417&seqNum=3
[2] https://accu.org/index.php/journals/1927 Refactoring Towards Seams in C++
[3] https://cute-test.com/guides/mocking-with-cute/
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> add c-unittest support
> --
>
> Key: DISPATCH-1568
> URL: https://issues.apache.org/jira/browse/DISPATCH-1568
> Project: Qpid Dispatch
> Issue Type: Improvement
> Components: Router Node
>Reporter: Nicolas
>Priority: Major
> Original Estimate: 336h
> Remaining Estimate: 336h
>
> Right now we are not using any framework for easily write c unit-test when
> developing (or later).
> The idea is to research available libraries, pick one and try to include in
> our workflow.
> preliminary candidates:
> [https://github.com/google/googletest]
> https://github.com/catchorg/Catch2
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] jdanekrh commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
jdanekrh commented on a change in pull request #684: WIP: (Work In Progress)
DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r380620641
##
File path: src/router_core/terminus.c
##
@@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term)
free_qdr_terminus_t(term);
}
+#ifdef TESTING
+__attribute__((weak))
+int mocked_vsnprintf(char *str, size_t size, const char *format, ...){return
-1;}
+#endif
// DISPATCH-1461: snprintf() is evil - it returns >= size on overflow. This
// wrapper will never return >= size, even if truncated. This makes it safe to
// do pointer & length arithmetic without overflowing the destination buffer in
// qdr_terminus_format()
-//
-static inline int safe_snprintf(char *str, size_t size, const char *format,
...)
-{
+// not static to be used unit-tested
+size_t safe_snprintf(char *str, size_t size, const char *format, ...) {
+// max size allowed must be INT_MAX (since vsnprintf reutrns an int)
+if (size == 0 || size > INT_MAX) {
+//TODO log a warning somewhere?
+return 0;
+}
+int max_possible_return_value = (int)(size - 1);
va_list ap;
va_start(ap, format);
+#ifdef TESTING
+int rc = mocked_vsnprintf(str, size, format, ap);
Review comment:
I was thinking about this and I don't like this much. I understand that in
C, there isn't many options to replace vsnprintf calls in tests, but this
"pollutes" the production code a lot.
In the C++ world, they seem to call these techniques "seams", [1] [2] [3].
Dispatch already uses the linker seam in two places, to mock
`qd_server_timeout` and `qd_timer_now`.
I generally like using the linker better, it seems cleaner to me.
[1] http://www.informit.com/articles/article.aspx?p=359417&seqNum=3
[2] https://accu.org/index.php/journals/1927 Refactoring Towards Seams in C++
[3] https://cute-test.com/guides/mocking-with-cute/
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
