Reputation: 628
I'm trying to make a program that does the following:
As command line arguments it receives paths to FIFO files. It is supposed to monitor those FIFOs using the epoll
API. Through the FIFOs it is guaranteed that only float numbers will be sent. The output of the program should be the sum of numbers send through each FIFO (the program stops when the write side closes all files).
Before I start with the actual code, here's a macro and a function you'll be seeing throughout the entire program:
#define iAssert(cond, msg) crash(cond, msg, __LINE__)
void crash(bool cond, char * msg, int line){
if(!cond){
perror(msg);
fprintf(stderr, "at line %d\n", line);
exit(EXIT_FAILURE);
}
}
It's just a simple asserting mechanism, irrelevant for the problem itself.
Anyway, first I fetch the number of FIFOs passed through command line arguments and create an epoll
file descriptor:
int numFifos = argc - 1;
int epollFileDesc = epoll_create(1);
iAssert(-1 != epollFileDesc, "epoll_create");
Then I create an array of fifo file descriptors and an array of sums, which in the following loop I initialize to zero.
int * fifoFileDescriptors = malloc(numFifos * sizeof(int));
iAssert(NULL != fifoFileDescriptors, "malloc1");
float * localSums = malloc(numFifos * sizeof(float));
iAssert(NULL != localSums, "malloc 2");
So far so good, I think. The following loop, apart from initializing the array of sums, opens FIFOs, fills the previous array of file descriptors and registers events.
for(int i = 0; i<numFifos; i++){
localSums[i] = 0.f;
int thisFd = open(argv[i+1], O_RDONLY | O_NONBLOCK);
iAssert(-1 != thisFd, "open");
fifoFileDescriptors[i] = thisFd;
FILE * thisFs = fdopen(thisFd, "r");
iAssert(NULL != thisFs, "fdopen");
DataPass registerThis;
registerThis.fifoIndex = i;
registerThis.file = thisFs;
struct epoll_event thisEvent;
thisEvent.events = 0;
thisEvent.events |= EPOLLIN;
thisEvent.data.ptr = (void *)®isterThis;
iAssert(-1 != epoll_ctl(epollFileDesc, EPOLL_CTL_ADD, thisFd, &thisEvent), "epoll_ctl");
}
The DataPass structure looks like this:
typedef struct{
int fifoIndex;
FILE * file;
}DataPass;
What I want is, as you can see, is to receive file streams instead of file descriptors because they're easier to read from. Apart from that I keep the index of the FIFO so I know which one is it.
After this I monitor for changes:
int nOpen = numFifos;
struct epoll_event events[MAX_EVENTS];
while(nOpen){
int active = epoll_wait(epollFileDesc, events, MAX_EVENTS, -1);
iAssert(-1 != active, "epoll_wait");
for(int i = 0; i<active; i++){
struct epoll_event thisEvent = events[i];
if(thisEvent.events & EPOLLIN){
DataPass * thisData = (DataPass *)thisEvent.data.ptr;
//fifo with index thisData->fifoIndex has sent a message
float x;
while(1 == fscanf(thisData->file, "%f", &x)){
localSums[thisData->fifoIndex] += x;
}
}else if (thisEvent.events & (EPOLLERR | EPOLLHUP)){
//need to close this connection
DataPass * thisData = (DataPass *)thisEvent.data.ptr;
iAssert(-1 != epoll_ctl(epollFileDesc, EPOLL_CTL_DEL, fifoFileDescriptors[thisData->fifoIndex], NULL), "epoll_ctl del");
fclose(thisData->file);
close(fifoFileDescriptors[thisData->fifoIndex]);
nOpen--;
}
}
}
The MAX_EVENTS
macro is defined to be 4.
After running this (and using a side program to make the fifos and send values through them) I get a segmentation fault which I've managed to track down to the fscanf
part. Even though I've tracked it down I still have no idea why it's causing it.
Any ideas?
Thanks.
Upvotes: 2
Views: 1452
Reputation: 25454
This line:
DataPass registerThis;
Declares a structure on the stack. As soon as you loop again, that memory will be overwritten with a new structure. Then you get a pointer to it:
thisEvent.data.ptr = (void *)®isterThis;
After the loop is over, this pointer does not point to anything meaningful.
To fix this, you need allocate that structure on the heap.
Upvotes: 1
Reputation: 40407
You are invoking undefined behavior by saving a pointer to a local variable past the validity of the stack in which it exists
for(int i = 0; i<numFifos; i++){
DataPass registerThis;
registerThis.file = thisFs;
thisEvent.data.ptr = (void *)®isterThis;
}
Don't export pointers to local variables and try to use them when they no longer exist. Allocate your storage in a more long-lived way.
Upvotes: 2