Code review

[Ebenezer]

I'd like to request comments on the code in the archive on
this page -- http://webEbenezer.net/build_integration.html .
As you may know, I'm working on an on line code generator
that writes C++ marshalling code based on user input. The
code generator is closed source, but the interface to the code
generator is open source. The code in the archive mentioned
above is the interface code. I use a three-tier architecture
as follows:

C++ Middleware Writer (CMW) -- this is a server ... closed source

C++ Middleware Writer Ambassador (CMWA) -- this is a server ... open
source

"direct" program -- this program runs once and exits ... open source

The two open source tiers are in the archive along with supporting
files. One of the files in the archive -- msg_shepherd.hh -- is
output from the CMW. That file is based on another file in the
archive called direct.mdl and some include files.

One thing that I expect will be brought up is that the software
mixes naming conventions. I haven't picked one and been
consistent with it as is sometimes advised here. I agree with
that advice for the most part, but have been lazy about following
it so far.

There's a mediateResponse function in the CMWA that
may result in comments. Some knowledge of the CMW
is needed --- if the returned transaction number is zero, the
value of the request result (reqResult) will be false. I can
only think of oddities like sun storms that would cause
this to be violated. But it wouldn't be difficult to add code
that checks if returnedTransactionNbr == 0 and reqResult
is true and throws an exception.

In the "direct" program the function that connects to the
ambassador uses 127.0.0.1. That needs to be worked
on.

The software in the archive has been tested on Linux and Windows.
I'd appreciate advice on improving the existing code as well as
suggestions of new functionality. Thanks in advance.


Brian Wood
Ebenezer Enterprises
http://webEbenezer.net
(651) 251-9384

 

[Ebenezer]

Why do you think anyone would spend time reviewing all your code for
free?  Reviewing a small class for free is probably not asking too much
but not entire an archive containing multiple non-trivial source files.

The way I see it, some people may not want to review any of it,
some may review a little of it and others may choose to review a
lot of it. I'll take what I can get. People thinking about
using the CMW may be willing to put in some effort to help me
improve it.

 

[Ebenezer]

The way I see it, some people may not want to review any of it,
some may review a little of it and others may choose to review a
lot of it.  I'll take what I can get.  People thinking about
using the CMW may be willing to put in some effort to help me
improve it.

I have a specific question that I've come across recently. It has to
with exceptions and control flow. This is from the CMWA.

cmwAmbassador::cmwAmbassador(char const* configfile) :
sendbuf(200000), buf(40000),

localsendbuf(4096),
#ifdef BIG_ENDIANS

byteOrder(most_significant_first),
#else

byteOrder(least_significant_first),
#endif

configData(configfile)
{
fd_set master; // master file descriptor list
fd_set read_fds; // temp file descriptor list for select()
FD_ZERO(&master);
FD_ZERO(&read_fds);

sock_type CMWfd = login();
sock_type listener = serverPrep(configData.portnumber);
// Add the listener to the master set
FD_SET(CMWfd, &master);
FD_SET(listener, &master);
#undef max // for Windows
int32_t fdmax = std::max(listener, CMWfd);

for (; {
read_fds = master;
if (select(fdmax + 1, &read_fds, NULL, NULL, NULL) == -1) {
if (EINTR == errno) {
continue;
}
throw failure("select() failed with errno of ") << errno;
}

for (int32_t sock = 0; sock <= fdmax; ++sock) {
if (FD_ISSET(sock, &read_fds)) {
if (sock == CMWfd) {
try {
if (!buf.GotPacket()) {
continue;
}
} catch(eof const& ex) {
closeSocket(CMWfd);
FD_CLR(CMWfd, &master);
flex_string<char> errorMsg = "CMW crashed before your
request was processed.";
transactions_t::iterator itr =
pendingTransactions.begin();
transactions_t::iterator endit =
pendingTransactions.end();
for (; itr != endit; ++itr) {
localsendbuf.sock_ = (*itr).second.sock;
msg_shepherd::Send(localsendbuf, false, errorMsg);
closeSocket((*itr).second.sock);
FD_CLR((*itr).second.sock, &master);
}
pendingTransactions.clear();

CMWfd = login();
fdmax = std::max(listener, CMWfd);
FD_SET(CMWfd, &master);
continue; // This line could go away if I
// move the code below up into the try.
}
int32_t localsock = mediateResponse();
if (localsock != 0) {
closeSocket(localsock);
FD_CLR(localsock, &master);
}
} else {
....


That's the way I have it currently. I've thought about changing it
to the following, picking up from the last for statement in the above:

for (int32_t sock = 0; sock <= fdmax; ++sock) {
if (FD_ISSET(sock, &read_fds)) {
if (sock == CMWfd) {
try {
if (!buf.GotPacket()) {
continue;
}
int32_t localsock = mediateResponse();
if (localsock != 0) {
closeSocket(localsock);
FD_CLR(localsock, &master);
}
} catch(eof const& ex) {
closeSocket(CMWfd);
FD_CLR(CMWfd, &master);
flex_string<char> errorMsg = "CMW crashed before your
request was processed.";
transactions_t::iterator itr =
pendingTransactions.begin();
transactions_t::iterator endit =
pendingTransactions.end();
for (; itr != endit; ++itr) {
localsendbuf.sock_ = (*itr).second.sock;
msg_shepherd::Send(localsendbuf, false, errorMsg);
closeSocket((*itr).second.sock);
FD_CLR((*itr).second.sock, &master);
}
pendingTransactions.clear();

CMWfd = login();
fdmax = std::max(listener, CMWfd);
FD_SET(CMWfd, &master);
}
} else {
....


That way the continue statement in the catch block goes away,
but I don't like having to move the call to mediateResponse from
outside the try to inside of it in this form. That function isn't
supposed to throw eof exceptions. Any advice on this?

 

[Ebenezer]

I'm posting the complete constructor this time. I've reworked
it since the previous post. The primary difference is pulling
out the check for activity from the CMW to be certain that is
always checked first. This version eliminates two of the
continue statements that were in the previous version.
I've added a comment to the post near the code where I have
a question. The question follows below.


cmwAmbassador::cmwAmbassador(char const* configfile) :
sendbuf(200000), buf(40000),
localsendbuf(4096),
#ifdef BIG_ENDIANS
byteOrder(most_significant_first),
#else
byteOrder(least_significant_first),
#endif

configData(configfile)
{
fd_set master; // master file descriptor list
fd_set read_fds; // temp file descriptor list for select()
FD_ZERO(&master);
FD_ZERO(&read_fds);

sock_type CMWfd = login();
sock_type listener = serverPrep(configData.portnumber);
// Add the listener to the master set
FD_SET(CMWfd, &master);
FD_SET(listener, &master);
#undef max // for Windows
int32_t fdmax = std::max(listener, CMWfd);

for (; {
read_fds = master;
if (select(fdmax + 1, &read_fds, NULL, NULL, NULL) == -1) {
if (EINTR == errno) {
continue;
}
throw failure("select() failed with errno of ") << errno;
}

if (FD_ISSET(CMWfd, &read_fds)) {
try {
if (buf.GotPacket()) {
int32_t localsock = mediateResponse();
if (localsock != 0) {
closeSocket(localsock);
FD_CLR(localsock, &master);
}
}
} catch(eof const& ex) {
closeSocket(CMWfd);
FD_CLR(CMWfd, &master);
flex_string<char> errorMsg = "CMW crashed before your request
was processed.";
transactions_t::iterator itr = pendingTransactions.begin();
transactions_t::iterator endit = pendingTransactions.end();
for (; itr != endit; ++itr) {
localsendbuf.sock_ = (*itr).second.sock;
msg_shepherd::Send(localsendbuf, false, errorMsg);
closeSocket((*itr).second.sock);
FD_CLR((*itr).second.sock, &master);
}
pendingTransactions.clear();

CMWfd = login(); // question relates to
this line and the next two.
FD_SET(CMWfd, &master);
fdmax = std::max(listener, CMWfd);
}
}

for (int32_t sock = 0; sock <= fdmax; ++sock) {
if (FD_ISSET(sock, &read_fds)) {
if (sock == listener) {
int newsock;
sockaddr_in amb_addr;
socklen_t amblen = sizeof(amb_addr);
if ((newsock = accept(listener, (sockaddr*) &amb_addr,
&amblen)) < 0) {
throw failure("accept() failed with errno of ") << errno;
}
FD_SET(newsock, &master);
if (newsock > fdmax) {
fdmax = newsock;
}
PersistentWrite(newsock, &byteOrder, 1);
} else {
if (sock != CMWfd) {
if (!mediateRequest(sock)) {
closeSocket(sock);
FD_CLR(sock, &master);
}
}
}
}
}
}
}


In that catch block I'm calling login() which can throw
so the ambassador could be abruptly terminated. I don't think
there's any other option though but to attempt to reestablish
the connection with the CMW though. Would you suggest adding
logging calls before and after the call to login? Do you call
functions in catch blocks that can throw?