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

Add RAW Socket support for ACE #2065

Closed
wants to merge 0 commits into from
Closed

Conversation

smithAchang
Copy link

I have completed the work of RAW Socket by referring ACE_ICMP_Socket & ACE_Sock_Dgram classes, these are provided by ACE long time ago.


I have tested the api of RAW Socket on Linux platforms, such as Ubuntu 20.04 and CentOS 7.
On windows platform , there exist some surprise maybe for OS limits, RAW_Socket_Test case can not pass through.

For considering the fact the applications using RAW Socket almost run on Unix* or Linux* platforms, this fail is trivial!

So I decide to launch the PR :)

Copy link
Member

@jwillemsen jwillemsen left a comment

Choose a reason for hiding this comment

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

Not a complete review, just a quick scan

ACE/ace/RAW_Socket.cpp Outdated Show resolved Hide resolved
};
#endif

#define SEND_EXCEPTION_RETURN() do{\
Copy link
Member

Choose a reason for hiding this comment

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

Prefix with ACE_ so that they don't clash with any other define

Copy link
Author

Choose a reason for hiding this comment

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

OK


}

static inline ssize_t using_common_recv(const ACE_RAW_SOCKET& raw, void *buf, size_t n, ACE_INET_Addr &addr, int flags)
Copy link
Member

Choose a reason for hiding this comment

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

Why static?

Copy link
Author

Choose a reason for hiding this comment

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

I want the function is inside the module and can not be referred by others

{
ACE_TRACE ("ACE_RAW_SOCKET::recv");

RECV_EXCEPTION_RETURN();
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the macros for this, makes it very hard to see what is checked

Copy link
Author

Choose a reason for hiding this comment

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

The checking conditions are unlikely and trivial, so need not care what is the exceptions .
The common macro trick can return the flow in place without any further if statement and can reduce the cyclomatic complexity

#endif /* ACE_HAS_SOCKADDR_MSG_NAME */
send_msg.msg_namelen = addr.get_size ();

#if defined (ACE_HAS_4_4BSD_SENDMSG_RECVMSG)
Copy link
Member

Choose a reason for hiding this comment

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

Use nullptr for pointers

Copy link
Author

Choose a reason for hiding this comment

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

Firstly I use nullptr according to the ACE developer guideline.
But when I migrated the RAW Socket into ACE 6.5.x version and compiled the ACE lib on CentOS7 with gcc4.8, I got some compiling error about nullptr, so I modify them to NULL keyword :(

if (this->get_handle () != ACE_INVALID_HANDLE)
return -1;

const int protocol_family = local.get_type ();
Copy link
Member

Choose a reason for hiding this comment

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

ACE uses east const, so int const

Copy link
Author

Choose a reason for hiding this comment

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

ok

* - Setting the protocol para to be IPPROTO_SCTP will filter all SCTP protocol packages with the destination is its bound address.
* It can form the basis of a user-space SCTP protocol stack in more general platforms.
*
* - Setting the protocol para to be IPPROTO_RAW will make it as a send only socket for any customized package formed from IP header to be send.
Copy link
Member

Choose a reason for hiding this comment

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

parameter

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean the 'para' word needed to be fixed ?

run_option_test ()
{

ACE_DEBUG ((LM_INFO, "%s begin to run ...\n", __func__));
Copy link
Member

Choose a reason for hiding this comment

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

Logging ascii is %C

Copy link
Author

Choose a reason for hiding this comment

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

OK

@jwillemsen jwillemsen removed their request for review April 6, 2023 14:31
@smithAchang
Copy link
Author

  • I left two issues found by codacy tool chain for I find codacy discovers the same issues about Sock_Dgram.cpp
  • I have used the cppcheck v2.10 tool localy obeying the advice from codacy team and encountered the same result.The codacy team considers it is the problem of cppcheck tool

cppcheck --enable=all --force $ACE_ROOT/ace/RAW_Socket.cpp


So I relaunch the review flow :)

@jwillemsen jwillemsen removed their request for review April 6, 2023 14:46
@jwillemsen
Copy link
Member

Please don't try to assign me as reviewer, my free time for unsponsored work is very limited, you have to wait until I have time, no idea when I have time for a full review.

@smithAchang
Copy link
Author

Please don't try to assign me as reviewer, my free time for unsponsored work is very limited, you have to wait until I have time, no idea when I have time for a full review.

OK :(

Can you assign other team member to review ?

@jwillemsen
Copy link
Member

When someone has time they will have a look, no guarantees, see https://github.com/DOCGroup/ACE_TAO/wiki/ACE-and-TAO-Commercial-support when you need guarantees, including @RemedyIT, the company I work for

@jwillemsen
Copy link
Member

We do appreciate your contribution! It is only that our time is very limited at this moment.

@smithAchang
Copy link
Author

I will add other features to ACE, so I cancel the PR based master branch!

I will launch the PR lately based on a special branch :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs review Needs to be reviewed
Development

Successfully merging this pull request may close these issues.

2 participants