Added support to QOS_XML_Handler to parse a string #2439

dczanella · 2021-02-23T12:45:23Z

Hi there,

I changed/added some files to include a new functionality by module QOS_XML_Handler to parse not only files, but sequence of characters. This code requires xerces-c library to compile as well QOS_XML_Handler. I tested using xerces-c version 3.2.2.
This code should be ported to OpenDDS version 3.13.3.
Cheers

… parsing xml from memory buffer

…cludes, tests), wrong class name in messages, removed virtual from inherited class and changed const char* to ACE_TCHAR in test

jwillemsen · 2021-02-23T12:49:20Z

dds/DCPS/QOS_XML_Handler/XML_File_Intf.h

@@ -4,6 +4,7 @@
 *
 *
 *  @author Marcel Smit (msmit@remedy.nl)
+ *  @2nd author Danilo C. Zanella (dczanella@gmail.com)


Use @author, @2nd is not a valid doxygten tag

jwillemsen · 2021-02-23T12:49:31Z

dds/DCPS/QOS_XML_Handler/XML_File_Intf.h

@@ -29,7 +31,8 @@ OPENDDS_BEGIN_VERSIONED_NAMESPACE_DECL
 namespace OpenDDS {
 namespace DCPS {

-  class OpenDDS_XML_QOS_Handler_Export QOS_XML_File_Handler
+  class OpenDDS_XML_QOS_Handler_Export QOS_XML_File_Handler :
+    virtual public QOS_XML_Handler


Does it need to be virtual?

jwillemsen · 2021-02-23T12:51:43Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+
+  DDS::ReturnCode_t
+  QOS_XML_Handler::get_datareader_qos(::DDS::DataReaderQos& dr_qos,
+                                            const ACE_TCHAR * profile_name,


Correct indentation of the arguments, all functions

jwillemsen · 2021-02-23T12:52:41Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

@@ -0,0 +1,228 @@
+
+#include "XML_MemBuf_Intf.h"
+//#include "ace/XML_Utils/XML_Typedefs.h"


Remove commented out includes

jwillemsen · 2021-02-23T12:53:16Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+    QOS_XML_Handler(),
+    res_(new XML::XML_Schema_Resolver<XML::Environment_Resolver>()),
+    eh_(new XML::XML_Error_Handler())
+  {


Initialise parser_ with 0

jwillemsen · 2021-02-23T12:53:55Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+
+      //// Check for errors
+      //if (eh_->getErrors())
+      //{


Remove commented out code

jwillemsen · 2021-02-23T12:54:24Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+        if (DCPS_debug_level > 1)
+        {
+          ACE_ERROR ((LM_ERROR,
+                ACE_TEXT ("QOS_XML_Stream_Handler::init - ")


Wrong classname

jwillemsen · 2021-02-23T12:54:49Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+    catch (...)
+    {
+      ACE_ERROR ((LM_ERROR,
+            ACE_TEXT ("QOS_XML_Stream_Handler::init - ")


wrong class name, check all messages

jwillemsen · 2021-02-23T12:55:31Z

tests/DCPS/QoS_XML/dumpMemBuf/qos_dumpMemBuf.cpp

+  try
+  {
+    // File name and profile name in respective file
+    const char * fileName = "qos.xml";


Should use ACE_TCHAR

jwillemsen · 2021-02-23T13:31:36Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.h

+#include "XML_Intf.h"
+#include "dds/DdsDcpsInfrastructureC.h"
+#include "OpenDDS_XML_QOS_Handler_Export.h"
+#include "ace/XML_Utils/XML_Typedefs.h"


Would prefer to extend the forward declared classes below and don't inlcude XM_Utils in the header, let us now expose any internals to the user

OK. I did it.

dczanella · 2021-02-24T09:05:43Z

Hi,

I need some help to complete the task. Some checks were not successful and it is not clear for me.
Thank

jwillemsen · 2021-02-24T11:15:43Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+
+  DDS::ReturnCode_t
+  QOS_XML_Handler::get_datawriter_qos(::DDS::DataWriterQos& dw_qos,
+                                            const ACE_TCHAR * profile_name,


Indentation strange

Hi
Could you be more specific?
Thanks

jwillemsen · 2021-02-24T11:17:16Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+              ACE_TEXT ("Exception message is: <%C>\n"),
+              message));
+        XMLString::release(&message);
+        return DDS::RETCODE_ERROR;


In case of a failure you don't disable ACE::debug

jwillemsen · 2021-02-24T11:17:59Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+        XMLString::release(&message);
+      }
+
+      DOMNode * clone = finalDoc_->importNode(ddsNode, true);


Can clone become 0?

jwillemsen · 2021-02-24T11:19:33Z

tests/DCPS/QoS_XML/dumpMemBuf/run_test.pl

+
+exit 0;
+
+#eval '(exit $?0)' && eval 'exec perl -S $0 ${1+"$@"}'


Remove commented out code

jwillemsen · 2021-02-24T12:14:13Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+  QOS_XML_Handler::get_datawriter_qos(::DDS::DataWriterQos& dw_qos,
+                                            const ACE_TCHAR * profile_name,
+                                            const ACE_TCHAR * topic_name)


Suggested change

QOS_XML_Handler::get_datawriter_qos(::DDS::DataWriterQos& dw_qos,

const ACE_TCHAR * profile_name,

const ACE_TCHAR * topic_name)

QOS_XML_Handler::get_datawriter_qos(::DDS::DataWriterQos& dw_qos,

const ACE_TCHAR * profile_name,

const ACE_TCHAR * topic_name)

jwillemsen · 2021-02-24T14:55:30Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+      if (DCPS_debug_level > 1)
+      {
+        char* message = XMLString::transcode(ddsNode->getNodeName());
+        ACE_ERROR ((LM_INFO,


Should be ACE_DEBUG for a debug message

Hi
I'll do it, but in QOS_XML_Loader.cpp it uses as I did.
Check following code:

DDS::ReturnCode_t
QOS_XML_Loader::init(const ACE_TCHAR* qos_profile)
{
if (!qos_profile)
{
if (DCPS_debug_level > 5)
{
ACE_ERROR((LM_ERROR,
ACE_TEXT("QOS_XML_Loader::init - ")
ACE_TEXT("Passed an empty qos_profile, returning.\n")));
}

Cheers

ACE_ERROR is for logging error messages, ACE_DEBUG for debug/info messages, both could be independently enabled/disabled at compilation time

Another question is:
How can i set the DCPS_debug_level variable? I set grater than 1, but is there some policy to do that?
Cheers

When this is an error you should use LM_ERROR instead of LM_INFO

Another question is:
How can i set the DCPS_debug_level variable? I set grater than 1, but is there some policy to do that?
Cheers

See http://download.objectcomputing.com/OpenDDS/OpenDDS-latest.pdf, chapter 7.6.1

jwillemsen · 2021-05-09T19:31:50Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+     if (ACE_OS::strcmp((*it)->name().c_str(), profileName) == 0) {
+       ACE_DEBUG((LM_DEBUG,
+                 ACE_TEXT("(%P|%t) DEBUG: QOS_XML_Handler::addQoSProfile - ")
+                 ACE_TEXT("Profile exists or profile name in use.\n")));


Log profile name

dczanella · 2021-05-10T13:41:19Z

It was the solution that I found, because those exceptions haven't ever been launched. Could you explain to me better how I can do that? I didn't find any ACE_ERROR/LM_ERROR problem, did you see anything?

…

On Sun, 9 May 2021 at 15:37, Johnny Willemsen ***@***.***> wrote: Instead of enabling/disabling ACE::debug() why not use DCPS_debug_level as check as we use more in the OpenDDS core? Also check that all error messages use ACE_ERROR/LM_ERROR — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJHCVVGQL4QSMK3QKCDRALTM3I7JANCNFSM4YCNOODA> .

-- Danilo C. Zanella

jwillemsen

Some minor comments, in general looks ok

jwillemsen · 2021-05-10T17:58:31Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+         ++it)
+    {
+      if (ACE_OS::strcmp((*it)->name().c_str(), profileName) == 0) {
+        ACE_DEBUG((LM_DEBUG,


This should be an error

jwillemsen · 2021-05-10T17:58:53Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+    }
+    ACE_DEBUG((LM_DEBUG,
+              ACE_TEXT("(%P|%t) ERROR: QOS_XML_Handler::delQoSProfile - ")
+              ACE_TEXT("Profile doesn't exists or wrong profile name.\n")));


jwillemsen · 2021-05-10T18:02:35Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+      {
+        if (DCPS_debug_level > 1)
+          {
+            ACE_DEBUG ((LM_DEBUG,


jwillemsen · 2021-05-18T15:02:24Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+      if (ACE_OS::strcmp((*it)->name().c_str(), profileName) == 0) {
+        if (DCPS_debug_level > 7)
+        {
+          ACE_DEBUG((LM_DEBUG,


Suggested change

ACE_DEBUG((LM_DEBUG,

ACE_ERROR((LM_ERROR,

jwillemsen · 2021-05-18T15:02:45Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+  DDS::ReturnCode_t
+  QOS_XML_Handler::addQoSProfileSeq(const dds::qosProfile_seq & profiles)
+  {
+


Remove empty lines at the top and bottom of this operation

jwillemsen · 2021-05-18T15:03:14Z

dds/DCPS/QOS_XML_Handler/XML_Intf.cpp

+
+    if (ACE_OS::strlen(profileName) == 0)
+    {
+      ACE_ERROR((LM_ERROR,


check debug level here for logging

jwillemsen · 2021-05-18T15:04:56Z

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp

+      {
+        if (DCPS_debug_level > 1)
+          {
+            ACE_DEBUG ((LM_DEBUG,


Suggested change

ACE_DEBUG ((LM_DEBUG,

ACE_ERROR ((LM_ERROR,

mitza-oci

It is confusing to me that this feature is for a "memory buffer." In terms the standard library uses this would be a "string," for example stringstream vs. fstream. The public API seems to be the init(const char*) method.

dczanella · 2021-05-25T15:04:16Z

I didn't catch that. What is your question? The idea is to manipulate xml from a sequence of chars besides a file. Cheers

…

On Tue, 25 May 2021 at 11:39, Adam Mitz ***@***.***> wrote: ***@***.**** requested changes on this pull request. It is confusing to me that this feature is for a "memory buffer." In terms the standard library uses this would be a "string," for example stringstream vs. fstream. The public API seems to be the init(const char*) method. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2439 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJHCVTCYDIVOIZ2DVV7YODTPOZDFANCNFSM4YCNOODA> .

-- Danilo C. Zanella

mitza-oci · 2021-05-25T20:02:34Z

I didn't catch that. What is your question? The idea is to manipulate xml from a sequence of chars besides a file.

I think it would be more clear for users if the new class was called something like QOS_XML_String_Handler to go with QOS_XML_File_Handler just like std::stringstream goes with std::fstream.

dczanella · 2021-05-25T21:40:20Z

Ok. I got it. I can do it.

…

On Tue, 25 May 2021 at 17:02, Adam Mitz ***@***.***> wrote: I didn't catch that. What is your question? The idea is to manipulate xml from a sequence of chars besides a file. I think it would be more clear for users if the new class was called something like QOS_XML_String_Handler to go with QOS_XML_File_Handler just like std::stringstream goes with std::fstream. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJHCVRQWO3MSPGFELTV6QLTPP66XANCNFSM4YCNOODA> .

-- Danilo C. Zanella

dczanella · 2021-05-25T21:44:52Z

Check if there are more changes needed. Thus I do it once. Cheers

…

On Tue, 25 May 2021 at 18:40, Danilo Zanella ***@***.***> wrote: Ok. I got it. I can do it. On Tue, 25 May 2021 at 17:02, Adam Mitz ***@***.***> wrote: > I didn't catch that. What is your question? The idea is to manipulate xml > from a sequence of chars besides a file. > > I think it would be more clear for users if the new class was called > something like QOS_XML_String_Handler to go with QOS_XML_File_Handler > just like std::stringstream goes with std::fstream. > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub > <#2439 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACJHCVRQWO3MSPGFELTV6QLTPP66XANCNFSM4YCNOODA> > . > -- Danilo C. Zanella

-- Danilo C. Zanella

mitza-oci · 2021-06-25T21:24:17Z

Check if there are more changes needed. Thus I do it once. Cheers

I think this should be a straightforward renaming. A side effect of doing another push to the branch is that it will re-run CI testing with the latest updates to how we've set up GitHub Actions.

tests/dcps_tests.lst

dczanella · 2021-07-08T14:02:30Z

Hi That is good news. Thanks Adam and Johnny, I have learned a lot with you. I would like to continue contributing with the project. I solved this small issue that was related to my job. If there are more things that I could help I'm wondering. Cheers

…

On Thu, 8 Jul 2021 at 10:43, Adam Mitz ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2439 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJHCVSDSMRJD3PIQMHR7CTTWWTRLANCNFSM4YCNOODA> .

-- Danilo C. Zanella

mitza-oci · 2021-07-08T16:16:21Z

CI checks are currently running on this pull request. Please take a look when those are done running and address any issues related to changes from this PR. There is already one failure in the "lint" check.

jwillemsen · 2021-07-09T13:45:25Z

Hi That is good news. Thanks Adam and Johnny, I have learned a lot with you. I would like to continue contributing with the project. I solved this small issue that was related to my job. If there are more things that I could help I'm wondering. Cheers

Maybe #2434, also in the QoS code, some new QoS settings can't be done through XML

dczanella added 4 commits February 23, 2021 09:43

Added support to QOS_XML_Handler to parse a memory buffer

06675d0

Added dumpMemBuf test to check new feature of QOS_XML_Handler library…

fa12c72

… parsing xml from memory buffer

Fixed initialization of some variables, argument spaces, comments (in…

0708908

…cludes, tests), wrong class name in messages, removed virtual from inherited class and changed const char* to ACE_TCHAR in test

Replacing includes by forward declarations

aa8057d

jwillemsen reviewed Feb 23, 2021

View reviewed changes

dds/DCPS/QOS_XML_Handler/XML_MemBuf_Intf.cpp Outdated

//// Check for errors

//if (eh_->getErrors())

//{

Copy link

Member

jwillemsen Feb 23, 2021

Choose a reason for hiding this comment

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

Remove commented out code

jwillemsen reviewed Feb 23, 2021

View reviewed changes

dczanella added 5 commits February 23, 2021 14:19

Fixing convertion error in MemBufInputSource

9c667eb

Changed to use reinterpret_cast

a3cbae1

Fixed type in std::ifstream call

2f530c6

Fixing types in init and add_search_path call

37a4e4e

Adding correct type conversion macros

0a421c7

Fixed indentation. Improve error check

e9551ba

dczanella requested a review from jwillemsen February 24, 2021 10:48

jwillemsen reviewed Feb 24, 2021

View reviewed changes

dczanella requested a review from jwillemsen February 24, 2021 12:26

Fixing MACROS for debugging and error.

231ba3e

jwillemsen reviewed Feb 24, 2021

View reviewed changes

jwillemsen reviewed May 9, 2021

View reviewed changes

dczanella added 6 commits May 10, 2021 14:33

Fixed indentation. Logged profile name.

ee4ea79

It already returns ACE_TCHAR

2f9d569

Fixing to give information at certain log levels only.

c99ff46

Made some corrections in the test procedure.

f79d69d

Removed trailing spaces

28aea12

Fixed string literal to ACE_TCHAR*

464ebce

dczanella requested a review from jwillemsen May 11, 2021 23:58

jwillemsen approved these changes May 18, 2021

View reviewed changes

Changing ACE_DEBUG to ACE_ERROR. Included check in DCPS_debug_level.

1cee89b

dczanella requested a review from jwillemsen May 25, 2021 13:49

jwillemsen approved these changes May 25, 2021

View reviewed changes

mitza-oci requested changes May 25, 2021

View reviewed changes

Renamed MemBuf to String in class and tests

ddadb53

dczanella requested a review from mitza-oci July 1, 2021 19:30

jwillemsen reviewed Jul 2, 2021

View reviewed changes

tests/dcps_tests.lst Outdated Show resolved Hide resolved

Added changes in dcps_tests due renaming

34f0ecb

mitza-oci approved these changes Jul 8, 2021

View reviewed changes

Fixed incorrect include guard

2aca473

mitza-oci changed the title ~~Added support to QOS_XML_Handler to parse a memory buffer~~ Added support to QOS_XML_Handler to parse a string Jul 9, 2021

mitza-oci merged commit 003261f into OpenDDS:master Jul 9, 2021

		@@ -0,0 +1,228 @@

		#include "XML_MemBuf_Intf.h"
		//#include "ace/XML_Utils/XML_Typedefs.h"


		exit 0;

		#eval '(exit $?0)' && eval 'exec perl -S $0 ${1+"$@"}'

Added support to QOS_XML_Handler to parse a string #2439

Added support to QOS_XML_Handler to parse a string #2439

Conversation

dczanella commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dczanella commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dczanella commented May 10, 2021 via email

jwillemsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitza-oci left a comment

Choose a reason for hiding this comment

dczanella commented May 25, 2021 via email

mitza-oci commented May 25, 2021

dczanella commented May 25, 2021 via email

dczanella commented May 25, 2021 via email

mitza-oci commented Jun 25, 2021

dczanella commented Jul 8, 2021 via email

mitza-oci commented Jul 8, 2021

jwillemsen commented Jul 9, 2021