A few weeks ago some students mentioned some coursework the had been given. They were given the C source code to a simple application and the assignment was to extend it in a number of ways. I would consider the quality of the provided code to be very poor and find it very worrying that this is being provided as learning material.
I won't name anyone in this article - it's not intended as a flame, it is intended that the people involved (students and lecturers) will learn from these comments.
The application is a simple chat program - run two instances of it and you can send messages between them. The code is here: messenger.c.
High level comments
A simple Makefile should really be provided. Something like:
CFLAGS=-pedantic -Wall -Wextra -std=gnu99
OBJS=messenger.o
BINARIES=messenger
.PHONY: clean
all: $(BINARIES)
%.d: %.c
$(SHELL) -ec '$(CC) -MM $(CFLAGS) $< | sed '\''s/\($*\)\.o[ :]*/\1.o $@ : /g'\'' > $@; [ -s $@ ] || rm -f $@'
-include $(OBJS:.o=.d)
clean:
@rm -vf $(OBJS) $(BINARIES) *.d
This would serve to document what compiler flags are needed and also
Makefiles are really important, so teaching them is
a Good Thing.
The provided code throws up warnings when compiling - code should
always be compiled with warnings turned on and if you don't get rid
of the warnings they will likely come back to bite you in the future.
At least, any warnings that cannot be fixed should be well understood
and documented. In this case, all that was needed was a simple
#include:
#include <string.h>
All the functions (except main()) and global variables should have been declared using the static keyword since they don't need to be exported for use by external code. Concepts such as data hiding are still just as important in C as in object orientated languages. Objects declared as static also get some extra compile time checking, allowing the compiler to warn you about things like redundent symbols that could be removed from the code.
There are far more appropriate IPC mechanisms available. For example, using PF_UNIX sockets would make things much easier since no locking and signalling would be required.
Many of the functions accept a pointer as a argument and never modify
the memory it points at. These arguments should be declared using the
const keyword. i.e.:
void GetLock(char *lockName)
Should be:
void GetLock(const char *lockName)
Detail
I'll go through the code line by line and make comments as we go...
#define true 1
#define false 0
In C, anything non-zero is considered to be true. Defining
the names true and false encourages people to
do something like:
if (foo == true)
This is wrong since it doesn't catch all truth values. Admittedly the
provided source code doesn't use these definitions in any comparison
operations, but defining them encourages misuse at a later date.
#define USE_ECHO 0 /*Must be set to 0 for proper display, which actually messes up the display afterward!*/
This comment indicates some missing functionality - setting the console attributes back on exit is pretty trivial (was this intentionally left as an exercise?)
char *lockFile="//tmp//default.lock"; /*A shared directory is suitable for real usage. Be careful to always erase this lock when not needed anymore*/
char *messageFile="//tmp//default.message.dat"; /*A shared directory is suitable for real usage*/
Why the double slashes? It'll work because they will be collapsed by the OS, but there's no reason for them.
pthread_mutex_t sendMessageMutex; /*May be needed somewhere...*/
That kind of comment just scares me - you mean you don't know if you need it or not? (Infact, it's not needed since no Posix locking is used). Also, if this had been declared as a static variable the compiler would have told you it was redundent.
int received;
This variable is modified by a signal handler, so it should be declared using the volatile keyword.
void StartListening(void); /*Declares the function. Actual function defined later*/
Why was this prototype needed? The function is already defined before use anyway. I consider prototyping local functions without good reason to be bad practice - it makes code maintenance harder.
void GetLock(char *lockName) /*Use a file as a lock*/
{
int d,cpt=0;
d=-1;
while (d==-1) {
d=open(lockName, O_WRONLY | O_CREAT | O_EXCL);/*O_EXCL: Refuse opening a file if already open*/
cpt++;
sleep(0); /*A delay is suitable here, but the sleep function only handles seconds...*/
}
close (d);
}
Spinning on a lock with no delay is very bad practice since it adversely affects the rest of the system. If you must do this, at least sleep for a few milliseconds using usleep(3) or select(2).
char command[MAX_TEXT_SIZE];
sprintf(command,"rm %s>/dev/null",lockName);/*Creates a command name. Output redirected to /dev/null*/
FILE *p=popen(command,"w");/*Call the command*/
This is extremely dangerous for two reasons - the sprintf(3) function does no length checking and could over run the end of the buffer (use snprintf(3) instead). More worryingly, deleting a file by executing the rm command could lead to catestrophic data loss since lockName is never sanitised to make sure it is shell safe. What's wrong with just calling the unlink(2) function?
char *openMessengersCommandLine="ps -edf|grep messenger| grep -v grep|tr -s ' ' | cut -d \" \" -f 2";
f=popen(openMessengersCommandLine,"r"); /*Get the pids of all the programs named Messenger*/
That's going to get you the PIDs of any processes running with the text "messenger" in their command line, even if they are unrelated to this software. You'll also miss any messenger processes which are running from a renamed executable - not good.
int ret_val=kill(pid,SIGUSR1); /*Sends a message to another Messenger process*/
Great, this is going to kill all those unrelated processes you just got PIDs for - really bad behaviour.
Also, in ANSI C, inline variable declarations are illegal - variables must be declared at the top of a code block. C99 allows this, but I still question teaching it as good practice.
while(!received)
sleep(0); /*A few milliseconds would be best*/
Again, spinning on a variable without a delay is really bad behaviour
and in this case is completely unnecessary. The received
variable is only updated on receipt of a signal, so it would be better
to use a function that blocks until the receipt of the signal, such as
select(2):
select(0, NULL, NULL, NULL, NULL);
ReleaseLock(messageFile); /*Actually just for deleting the message. No lock functionality here*/
Abusing a function to do something it wasn't intended to do is extremely bad practice - if the behaviour of the function changes then this will break. e.g. the locking mechanism may be completely replaced at a future time, resulting in ReleaseLock() nolonger deleting the file. Just use the unlink(2) function.
strcat(message,myMessage); /*Appends message to user name*/
No length checking done - this could overrun the buffer.
Slashdot It!
| Submit a comment |
|---|
| Comment submitted by finnw at 13:05 on Sunday 10th June, 2007 |
| Steve, I think your argument against the true/false macros is bogus. I agree that writing "if (b == true)" is silly, but the macros don't force anyone to do that. |
| Comment submitted by Kat at 12:32 on Friday 4th May, 2007 |
| Good article :) Though I wanted to say (again because you ran off :P) that several of the things you mentioned (the mutex, the lock, something else on mw) were put in or left out as part of the actual coursework questions. We had to implement threading in |