[newbie][request for comment] A simple socket based IRC bot for *nix and win32
Leslie Kis-Adam
I'm a beginner in network programming, and I made a simple IRC bot.
I've tried to make it as portable as possible, and as the result of that
effort it runs on both *nix and win32 platforms. For debugging purposes
it connects to a Quakenet IRC server on port 6667, and registers as
dfighterbot.
Here are the sources in a tar file:
http://filebin.ca/wqrqyb/dfighterbotrev6.tar
Please comment my code, so I can learn from my mistakes/inefficient
solutions. Thank you very much.
Kind regards,
Leslie Kis-Adam
David Schwartz
> Please comment my code, so I can learn from my mistakes/inefficient
> solutions. Thank you very much.
Your 'GetIRCmessage' function is really poor.
1) It can't decide whether it's using blocking I/O or non-blocking I/
O. It calls 'select', but then does a blocking 'recv'.
2) It exits on a socket error even if that socket error is non-fatal
(like ENOBUFS).
3) It uses MSG_PEEK, followed by 'recv'. There is no reason for this.
Just receive as much data as possible and parse it afterwards.
(Keeping the leftover for the next pass.)
4) If your call to 'recv' gets half a message, you will break
horribly. This can easily happen.
5) Why call a function with a timeout in an endless loop that does
nothing else?
Other smaller issues:
1) Long 'strcmp' chains are considered ugly. Use some kind of lookup
table, perhaps with a hash.
2) You have no loopback breaking. If a bot is ever going to send a
PRIVMSG to someone, it must make sure it doesn't cause another bot to
send it back a PRIVMSG that generates a loop. If two bots get into
deadly embrace like this, they can be flooded if the server, banned
for abuse, or the like.
Other than that, not bad. The only really serious issues that I saw
are the use of MSG_PEEK and the assumption that a call to 'recv' will
return at least one protocol data unit.
DS
Ulrich Eckhardt
I didn't go too much into the network-related stuff, but here a few things I
noticed:
1. Inconsistent indention
This makes code harder to read. I personally prefer spaces to tabs (because
they look the same on any system) and four spaces for indention, but much
more important is that it is consistent.
2. No include guards
The few header files you used don't have any include guards. Might be
overkill for the size of the project though.
3. System-specific headers
You have (at least) two places where you include headers for Berkeley
sockets or WinSock depending on the preprocessor. One of those places is in
a header, which is lateron included in the same sourcefile where another
such place is. Just do it in a header.
4. Preprocessor include structure
As mentioned above, you have two places where you have:
#ifndef WIN32
#include <this.h>
#else
#include <that.h>
#endif
I'd change two things there, firstly remove the negation and secondly indent
this code, too:
#ifdef WIN32
# include <that.h>
#else
# include <this.h>
#endif
Removing the negation makes it easier to understand just as indenting the
code. Note that I left the # at the leftmost position and that I used two
spaces and not four for indention. This is supposed to make clear that the
preprocessor doesn't follow the same scoping rules as the C code lateron.
5. Inconsistent naming
There is a function like GetFoo() and another like doSomething(). Rename
either to getFoo() or DoSomething().
6. 'const'
Using 'char const*' when passing a string documents that you are not going
to modify the string.
7. unchecked sprintf()
Prefer snprintf() to avoid buffer overruns. Handle errors.
8. Inconsistent or missing error handling
There are some functions that simply call exit() while others return an
errorcode. Without knowing which function does what, it's hard to guess
where you simply lack error checking and where errors are handled by
exiting.
Further, looking at parseBotCommand(), I see that this function doesn't seem
to handle the case that an unknown command is received. Also, it always
returns zero. Going up one level to parseprivmsg(), you also see that this
function also always returns zero. However, it doesn't matter, because the
returnvalue from parseprivmsg() is never even checked!
9. Function documentation
Whenever a function returns a resource (e.g. a socket handle or some memory
allocated with malloc()) it should be documented that and how the caller
needs to release that resource to avoid memory leaks. Similarly when a
resource is transferred to a function.
Further, e.g. in main(), there is code to daemonize (I think that's what it
is) the bot, using a call to fork(). The reason for this call is completely
undocumented. Similarly the code to init WinSock. Suggestion:
#ifdef WIN32
initWinsock();
#endif
#ifndef WIN32
daemonizeProcess();
#endif
In the functions, you can then document the rationale for doing things a bit
without cluttering main() with too much code.
10. Magic numbers
Code like:
char nick[513];
requires some knowledge in order to understand. This is mainly a problem
with code maintenance. Also, fixed-size arrays might pose a problem because
they can overflow. BTW: there is a #define BUFFER 513, which might be the
one you meant. However, it's impossible to say because it is not documented
what that macro should mean...
Otherwise, I personally wouldn't say that it's the work of a newbie. It is
neatly structured into a handful of sourcefiles that are separated by their
tasks. Also no typical beginners' errors that jump out, except for the
sprintf() one.
Uli