1.1.7 rlm_detail locking is broken
Hello, I recently installed freeradius and have it forwarding accounting information. I noticed that there were a lot of failed locks in the logs, so I began to investigate. In rlm_detail, I found this loop for locking: do { if (inst->locking) { lseek(outfd, 0L, SEEK_SET); if (rad_lockfd_nonblock(outfd, 0) < 0) { close(outfd); tv.tv_sec = 0; tv.tv_usec = 25000; select(0, NULL, NULL, NULL, &tv); lock_count++; } else { DEBUG("rlm_detail: Acquired filelock, tried %d time(s)", lock_count + 1); locked = 1; } } } while (!locked && inst->locking && lock_count < 80); This loop, if it fails on the first lock, might as well not try again. Why? Because it closes the file descriptor when it fails. All of the following calls just come back with 'bad file descriptor'. The oddest part about this is that I had found this thread: http://osdir.com/ml/freeradius.devel/2005-03/msg00112.html It looks like the locking should work perfectly! But it seems these changes never found their way into the 1.x series, despite having occurred two years ago. I poked around in CVS and found revision 1.43 which is when the corrected locking was added. I took a diff of revision 1.42 and 1.43, and applied it by hand to 1.1.7. I haven't had any complaints from freeradius about locking issues since. I have attached the patch. Is there any reason this never found its way into the 1.x series? I see it's in the CVS head. --- src/modules/rlm_detail/rlm_detail.c.orig 2007-11-21 14:53:45.000000000 -0800 +++ src/modules/rlm_detail/rlm_detail.c 2007-11-26 11:41:58.000000000 -0800 @@ -271,25 +271,24 @@ *p = '/'; } /* else there was no directory delimiter. */ - /* - * Open & create the file, with the given permissions. - */ - if ((outfd = open(buffer, O_WRONLY | O_APPEND | O_CREAT, - inst->detailperm)) < 0) { - radlog(L_ERR, "rlm_detail: Couldn't open file %s: %s", - buffer, strerror(errno)); - return RLM_MODULE_FAIL; - } - - /* - * If we're not using locking, we'll just pass straight though - * the while loop. - * If we fail to aquire the filelock in 80 tries (approximately - * two seconds) we bail out. - */ locked = 0; lock_count = 0; do { + /* + * Open & create the file, with the given + * permissions. + */ + if ((outfd = open(buffer, O_WRONLY | O_APPEND | O_CREAT, + inst->detailperm)) < 0) { + radlog(L_ERR, "rlm_detail: Couldn't open file %s: %s", + buffer, strerror(errno)); + return RLM_MODULE_FAIL; + } + + /* + * If we fail to aquire the filelock in 80 tries + * (approximately two seconds) we bail out. + */ if (inst->locking) { lseek(outfd, 0L, SEEK_SET); if (rad_lockfd_nonblock(outfd, 0) < 0) { @@ -298,15 +297,36 @@ tv.tv_usec = 25000; select(0, NULL, NULL, NULL, &tv); lock_count++; - } else { - DEBUG("rlm_detail: Acquired filelock, tried %d time(s)", - lock_count + 1); - locked = 1; + continue; + } + + /* + * The file might have been deleted by + * radrelay while we tried to acquire + * the lock (race condition) + */ + if (fstat(outfd, &st) != 0) { + radlog(L_ERR, "rlm_detail: Couldn't stat file %s: %s", + buffer, strerror(errno)); + close(outfd); + return RLM_MODULE_FAIL; } + if (st.st_nlink == 0) { + DEBUG("rlm_detail: File %s removed by another program, retrying", + buffer); + close(outfd); + lock_count = 0; + continue; + } + + DEBUG("rlm_detail: Acquired filelock, tried %d time(s)", + lock_count + 1); + locked = 1; } - } while (!locked && inst->locking && lock_count < 80); + } while (inst->locking && !locked && lock_count < 80); - if (!locked && inst->locking && lock_count >= 80) { + if (inst->locking && !locked) { + close(outfd); radlog(L_ERR, "rlm_detail: Failed to aquire filelock for %s, giving up", buffer); return RLM_MODULE_FAIL;
Brian De Wolf wrote:
I recently installed freeradius and have it forwarding accounting information. I noticed that there were a lot of failed locks in the logs, so I began to investigate. In rlm_detail, I found this loop for locking: ... It looks like the locking should work perfectly! But it seems these changes never found their way into the 1.x series, despite having occurred two years ago. I poked around in CVS and found revision 1.43 which is when the corrected locking was added. I took a diff of revision 1.42 and 1.43, and applied it by hand to 1.1.7. I haven't had any complaints from freeradius about locking issues since.
Thanks. I've committed the patch.
I have attached the patch. Is there any reason this never found its way into the 1.x series? I see it's in the CVS head.
Some things just get missed. Alan DeKok.
Brian De Wolf wrote:
The oddest part about this is that I had found this thread: http://osdir.com/ml/freeradius.devel/2005-03/msg00112.html
It looks like the locking should work perfectly! But it seems these changes never found their way into the 1.x series, despite having occurred two years ago.
This was a long time ago therefore I'm not sure I remember correctly. In my memories the solution was in two parts: one algorithm for rlm_detail and an other for radrelay.
I poked around in CVS and found revision 1.43 which is when the corrected locking was added. I took a diff of revision 1.42 and 1.43, and applied it by hand to 1.1.7. I haven't had any complaints from freeradius about locking issues since.
I think this is only half of the fix. The error messages have disappeared but I suspect there're still race conditions without the corresponding changes to radrelay.c.
I have attached the patch. Is there any reason this never found its way into the 1.x series? I see it's in the CVS head.
Probably because radrelay is deprecated in the 2.0 series. The file "radrelay.c" was never properly fixed in CVS head. I must admit I completely forgot about it after so much time. The radrelay functionality is now integrated into the server core, where you can find the second part of the locking algorithm. See src/main/detail.c. -- Nicolas Baradakis
Oh, how very annoying... On 11/29/07 16:38, Nicolas Baradakis wrote:
This was a long time ago therefore I'm not sure I remember correctly. In my memories the solution was in two parts: one algorithm for rlm_detail and an other for radrelay.
After looking back at that thread, I see you are correct.
I think this is only half of the fix. The error messages have disappeared but I suspect there're still race conditions without the corresponding changes to radrelay.c.
I think you might also be right on this as well...
Probably because radrelay is deprecated in the 2.0 series. The file "radrelay.c" was never properly fixed in CVS head. I must admit I completely forgot about it after so much time.
The radrelay functionality is now integrated into the server core, where you can find the second part of the locking algorithm. See src/main/detail.c.
Well, since the 2.0 series doesn't have a stable yet, this is still somewhat of an issue. The rlm_detail/radrelay locking in 1.1.7 itself is horribly broken, anyway. One could argue that with that interaction I KNEW when I was losing logs, though. I'll look into modifying radrelay to observe the modified algorithm in the old mail thread. Using src/main/detail.c would feel more like a complete rewrite, whereas I should only really need to tweak where/how it opens files. Expect a patch, soon enough...
On 11/29/07 17:46, Brian De Wolf wrote:
Expect a patch, soon enough...
Here's the patch, as promised. Now, instead of radrelay's previous behavior, radrelay will attempt to open and lock <detail>.work. If it can, it will then read/transmit all of the entries and delete <detail>.work. If it can't lock, it will simply try locking later. If it doesn't exist, it will check if <detail> has entries and if so it will move <detail> to <detail>.work (and make a new <detail>). It seems to move faster, but that's because I removed some of the sleeps to make sure writing had completed since that should be handled by the locks now. Luckily, my relay destinations aren't production yet, so I can keep testing with a decent load. How does it look? --- src/main/radrelay.c.orig 2007-03-16 06:22:03.000000000 -0700 +++ src/main/radrelay.c 2007-12-03 12:24:46.000000000 -0800 @@ -202,8 +202,6 @@ /* * Read one request from the detail file. - * Note that the file is locked during the read, and that - * we return *with the file locked* if we reach end-of-file. * * STATE_EMPTY: Slot is empty. * STATE_BUSY1: Looking for start of a detail record (timestamp) @@ -219,8 +217,6 @@ char key[32], val[32]; int skip; long fpos; - int x; - unsigned int i = 0; /* Never happens */ if (r_req->state == STATE_FULL) @@ -230,21 +226,7 @@ r_req->state = STATE_BUSY1; } - /* - * Try to lock the detail-file. - * If lockf is used we want to lock the _whole_ file, hence the - * fseek to the start of the file. - */ fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - do { - x = rad_lockfd_nonblock(fileno(fp), 0); - if (x == -1) - ms_sleep(100); - } while (x == -1 && i++ < 20); - - if (x == -1) - return 0; redo: s = NULL; @@ -359,11 +341,6 @@ return EOF; } - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); - return 0; } @@ -537,11 +514,13 @@ if (rename(from, to) < 0) return -1; + oldmask = umask(0); if ((n = open(from, O_CREAT|O_RDWR, st.st_mode)) >= 0) close(n); umask(oldmask); + return 0; } @@ -565,13 +544,13 @@ struct relay_request *r; struct timeval tv; struct relay_stats stats; + struct stat st; fd_set readfds; char work[1030]; - time_t now, uptime, last_rename = 0; - int i, n; + time_t now, uptime; + int i, n, x; int state = STATE_RUN; int id; - long fpos; strNcpy(work, r_args->detail, sizeof(work) - 6); strcat(work, ".work"); @@ -607,19 +586,47 @@ if (got_sigterm) state = STATE_SHUTDOWN; /* - * Open detail file - if needed, and if we can. + * Try to open our work file first. If not, check if the + * actual detail file has stuff in it. If so, open it, + * move it to the work file, and replace it with an empty + * file. Then keep the file handle for locking/reading. */ if (state == STATE_RUN && fp == NULL) { - if ((fp = fopen(work, "r+")) != NULL) - state = STATE_BACKLOG; - else - fp = fopen(r_args->detail, "r+"); - if (fp == NULL) { - fprintf(stderr, "%s: Unable to open detail file - %s\n", progname, r_args->detail); - perror("fopen"); - return; + if ((fp = fopen(work, "r+")) == NULL) { + if(!stat(r_args->detail, &st)) { + if(st.st_size > 0) { + detail_move(r_args->detail, work); + continue; + } + } } + /* + * Try to lock the detail-file. + * If lockf is used we want to lock the _whole_ file, hence the + * fseek to the start of the file. + * + * If we can't lock the detail file, we close it and + * will try it again on the next iteration. Note that + * we will never unlock the file, on purpose, to stop + * rlm_detail from reading it. + */ + + if(fp) { + fseek(fp, 0L, SEEK_SET); + do { + x = rad_lockfd_nonblock(fileno(fp), 0); + if (x == -1) + ms_sleep(100); + } while (x == -1 && i++ < 20); + + if (x == -1) { + fclose(fp); + fp = NULL; + } else { + state = STATE_BACKLOG; + } + } } /* @@ -640,43 +647,6 @@ state = STATE_CLOSE; break; } - - /* - * End of file. See if the file has - * any size, and if we renamed less - * than 10 seconds ago or not. - */ - now = time(NULL); - if (ftell(fp) == 0 || now < last_rename + 10) { - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); - break; - } - last_rename = now; - - /* - * We rename the file to <file>.work - * and create an empty new file. - */ - if (detail_move(r_args->detail, work) == 0) { - if (debug_flag > 0) - fprintf(stderr, "Moving %s to %s\n", - r_args->detail, work); - /* - * rlm_detail might still write - * something to <detail>.work if - * it opens <detail> before it is - * renamed (race condition) - */ - ms_sleep(1000); - state = STATE_BACKLOG; - } - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); } while(0); if (r_args->records_print && state == STATE_RUN){ stats.records_read++;
Brian De Wolf wrote:
Here's the patch, as promised.
Many thanks.
Now, instead of radrelay's previous behavior, radrelay will attempt to open and lock <detail>.work. If it can, it will then read/transmit all of the entries and delete <detail>.work. If it can't lock, it will simply try locking later.
The new algorith doesn't require a non-blocking lock in a loop. After the file is renamed rlm_detail will not try to lock it anymore.
If it doesn't exist, it will check if <detail> has entries and if so it will move <detail> to <detail>.work (and make a new <detail>).
rlm_detail will create the new detail file. I think the function detail_move() is not needed anymore. Just use a normal rename().
It seems to move faster, but that's because I removed some of the sleeps to make sure writing had completed since that should be handled by the locks now. Luckily, my relay destinations aren't production yet, so I can keep testing with a decent load.
That's good news.
--- src/main/radrelay.c.orig 2007-03-16 06:22:03.000000000 -0700 +++ src/main/radrelay.c 2007-12-03 12:24:46.000000000 -0800
[...]
@@ -537,11 +514,13 @@ if (rename(from, to) < 0) return -1;
+ oldmask = umask(0); if ((n = open(from, O_CREAT|O_RDWR, st.st_mode)) >= 0) close(n); umask(oldmask);
+ return 0; }
This is only cosmetics.
@@ -607,19 +586,47 @@ if (got_sigterm) state = STATE_SHUTDOWN;
/* - * Open detail file - if needed, and if we can. + * Try to open our work file first. If not, check if the + * actual detail file has stuff in it. If so, open it, + * move it to the work file, and replace it with an empty + * file. Then keep the file handle for locking/reading. */
Maybe re-use the comments from Alan in src/main/detail.c: /* * Open detail.work first, so we don't lose * accounting packets. It's probably better to * duplicate them than to lose them. * * Note that we're not writing to the file, but * we've got to open it for writing in order to * establish the lock, to prevent rlm_detail from * writing to it. */
+ if ((fp = fopen(work, "r+")) == NULL) {
/* * Try reading the detail file. If it * doesn't exist, we can't do anything. * * Doing the stat will tell us if the file * exists, even if we don't have permissions * to read it. */
+ if(!stat(r_args->detail, &st)) { + if(st.st_size > 0) { + detail_move(r_args->detail, work);
As said above, I think you could just use rename() and then you don't need to check st_size > 0.
+ continue;
I have a question about this "continue": please check that radrelay will not use 100% cpu when there is no detail file to read. (as many of the sleep() are gone)
+ } + } }
+ /* + * Try to lock the detail-file. + * If lockf is used we want to lock the _whole_ file, hence the
Please no line of more than 80 characters in comments.
+ * fseek to the start of the file. + * + * If we can't lock the detail file, we close it and + * will try it again on the next iteration. Note that + * we will never unlock the file, on purpose, to stop + * rlm_detail from reading it. + */
The last part is confusing. rlm_detail must not try to acquire the lock anymore after the file is renamed to detail.work.
+ + if(fp) { + fseek(fp, 0L, SEEK_SET); + do { + x = rad_lockfd_nonblock(fileno(fp), 0); + if (x == -1) + ms_sleep(100); + } while (x == -1 && i++ < 20); + + if (x == -1) { + fclose(fp); + fp = NULL; + } else { + state = STATE_BACKLOG; + }
No need to lock in a loop, so you can simplify that part. The lock is only there to catch a corner case where the rename() happened while the current rlm_detail was still writing in the file. (but new rlm_detail execution will create and use a new file) -- Nicolas Baradakis
Thanks for the quick response! On 12/03/07 16:40, Nicolas Baradakis wrote:
Now, instead of radrelay's previous behavior, radrelay will attempt to open and lock <detail>.work. If it can, it will then read/transmit all of the entries and delete <detail>.work. If it can't lock, it will simply try locking later.
The new algorith doesn't require a non-blocking lock in a loop. After the file is renamed rlm_detail will not try to lock it anymore.
Well, I thought the lock was not to prevent rlm_detail from writing, but to prevent radrelay from reading before rlm_detail was finished writing.
If it doesn't exist, it will check if <detail> has entries and if so it will move <detail> to <detail>.work (and make a new <detail>).
rlm_detail will create the new detail file. I think the function detail_move() is not needed anymore. Just use a normal rename().
I left that in there because there was an exit condition if radrelay couldn't open either the work file or the original file. I eventually took out that exit condition altogether, but didn't take out the file creation for some reason...
This is only cosmetics.
Oops...I commented out the file creation, but as mentioned above, put it back in for the time being. Out it goes again...
Maybe re-use the comments from Alan in src/main/detail.c:
That sounds a lot better than mine, so sure!
+ if(!stat(r_args->detail, &st)) { + if(st.st_size > 0) { + detail_move(r_args->detail, work);
As said above, I think you could just use rename() and then you don't need to check st_size > 0.
I was thinking about that, but I didn't want to make a loop where <detail> kept appearing as an empty file and I kept moving it away.
+ continue;
I have a question about this "continue": please check that radrelay will not use 100% cpu when there is no detail file to read. (as many of the sleep() are gone)
I used continue so that, once <detail> was renamed to <detail>.work, the loop would start over and open <detail>.work. However, this makes assumptions that this block of code will always be at the beginning of the loop, so I'm going to change 'continue' to the fopen that I was taking the long way to get to.
Please no line of more than 80 characters in comments.
Oops, sorry about that. My co-worker just showed me the "gq" command in vim, though, so it shouldn't happen any more...
+ * fseek to the start of the file. + * + * If we can't lock the detail file, we close it and + * will try it again on the next iteration. Note that + * we will never unlock the file, on purpose, to stop + * rlm_detail from reading it. + */
The last part is confusing. rlm_detail must not try to acquire the lock anymore after the file is renamed to detail.work.
Oops, that's not confusing, that's just wrong altogether :). I meant to say lock and write. However, this seems to be well explained by the comment from src/main/detail.c.
No need to lock in a loop, so you can simplify that part. The lock is only there to catch a corner case where the rename() happened while the current rlm_detail was still writing in the file. (but new rlm_detail execution will create and use a new file)
Good point, revised. I thought about making this a blocking lock (rlm_detail should release it quickly, I would hope) but it's probably better to let radrelay run through the other parts of the loop while waiting. I've attached a revised patch. I've dropped it in, and it works as expected. It doesn't peg the CPU at all, I assume it spends most of its time in the select's waiting on network traffic. I almost want to add some more sleeping though, it's hard to see the files appear and disappear, they move so fast... :) I'll see how it responds during the day tomorrow, load is lighter right now (wireless users). --- src/main/radrelay.c.orig 2007-03-16 06:22:03.000000000 -0700 +++ src/main/radrelay.c 2007-12-03 18:31:20.000000000 -0800 @@ -140,7 +140,6 @@ int read_one(FILE *fp, struct relay_request *req); int do_recv(struct relay_misc *r_args); int do_send(struct relay_request *r, char *secret); -int detail_move(char *from, char *to); void loop(struct relay_misc *r_args); int find_shortname(char *shortname, char **host, char **secret); void usage(void); @@ -202,8 +201,6 @@ /* * Read one request from the detail file. - * Note that the file is locked during the read, and that - * we return *with the file locked* if we reach end-of-file. * * STATE_EMPTY: Slot is empty. * STATE_BUSY1: Looking for start of a detail record (timestamp) @@ -219,8 +216,6 @@ char key[32], val[32]; int skip; long fpos; - int x; - unsigned int i = 0; /* Never happens */ if (r_req->state == STATE_FULL) @@ -230,21 +225,7 @@ r_req->state = STATE_BUSY1; } - /* - * Try to lock the detail-file. - * If lockf is used we want to lock the _whole_ file, hence the - * fseek to the start of the file. - */ fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - do { - x = rad_lockfd_nonblock(fileno(fp), 0); - if (x == -1) - ms_sleep(100); - } while (x == -1 && i++ < 20); - - if (x == -1) - return 0; redo: s = NULL; @@ -359,11 +340,6 @@ return EOF; } - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); - return 0; } @@ -523,30 +499,6 @@ } /* - * Rename a file, then recreate the old file with the - * same permissions and zero size. - */ -int detail_move(char *from, char *to) -{ - struct stat st; - int n; - int oldmask; - - if (stat(from, &st) < 0) - return -1; - if (rename(from, to) < 0) - return -1; - - oldmask = umask(0); - if ((n = open(from, O_CREAT|O_RDWR, st.st_mode)) >= 0) - close(n); - umask(oldmask); - - return 0; -} - - -/* * Open detail file, collect records, send them to the * remote accounting server, yadda yadda yadda. * @@ -567,11 +519,10 @@ struct relay_stats stats; fd_set readfds; char work[1030]; - time_t now, uptime, last_rename = 0; + time_t now, uptime; int i, n; int state = STATE_RUN; int id; - long fpos; strNcpy(work, r_args->detail, sizeof(work) - 6); strcat(work, ".work"); @@ -607,19 +558,40 @@ if (got_sigterm) state = STATE_SHUTDOWN; /* - * Open detail file - if needed, and if we can. + * Open detail.work first, so we don't lose + * accounting packets. It's probably better to + * duplicate them than to lose them. + * + * Note that we're not writing to the file, but + * we've got to open it for writing in order to + * establish the lock, to prevent rlm_detail from + * writing to it. */ if (state == STATE_RUN && fp == NULL) { - if ((fp = fopen(work, "r+")) != NULL) - state = STATE_BACKLOG; - else - fp = fopen(r_args->detail, "r+"); - if (fp == NULL) { - fprintf(stderr, "%s: Unable to open detail file - %s\n", progname, r_args->detail); - perror("fopen"); - return; + if ((fp = fopen(work, "r+")) == NULL) { + /* + * Try moving the detail file. If it + * doesn't exist, we can't do anything. + */ + if(rename(r_args->detail, work) != -1) + fp = fopen(work, "r+"); } + + if(fp) { + /* + * Try to lock the detail-file. If lockf is + * used we want to lock the _whole_ file, hence + * the fseek to the start of the file. + */ + fseek(fp, 0L, SEEK_SET); + if(rad_lockfd_nonblock(fileno(fp), 0) == -1) { + fclose(fp); + fp = NULL; + } else { + state = STATE_BACKLOG; + } + } } /* @@ -640,43 +612,6 @@ state = STATE_CLOSE; break; } - - /* - * End of file. See if the file has - * any size, and if we renamed less - * than 10 seconds ago or not. - */ - now = time(NULL); - if (ftell(fp) == 0 || now < last_rename + 10) { - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); - break; - } - last_rename = now; - - /* - * We rename the file to <file>.work - * and create an empty new file. - */ - if (detail_move(r_args->detail, work) == 0) { - if (debug_flag > 0) - fprintf(stderr, "Moving %s to %s\n", - r_args->detail, work); - /* - * rlm_detail might still write - * something to <detail>.work if - * it opens <detail> before it is - * renamed (race condition) - */ - ms_sleep(1000); - state = STATE_BACKLOG; - } - fpos = ftell(fp); - fseek(fp, 0L, SEEK_SET); - rad_unlockfd(fileno(fp), 0); - fseek(fp, fpos, SEEK_SET); } while(0); if (r_args->records_print && state == STATE_RUN){ stats.records_read++;
Brian De Wolf wrote:
I've attached a revised patch. I've dropped it in, and it works as expected. It doesn't peg the CPU at all, I assume it spends most of its time in the select's waiting on network traffic. I almost want to add some more sleeping though, it's hard to see the files appear and disappear, they move so fast... :) I'll see how it responds during the day tomorrow, load is lighter right now (wireless users).
Many thanks for the second patch, and thanks for doing the testing, too. Your efforts are lowering the amount of work on our side to a minimum. I've reviewed your last patch and it looks ok to me. If Alan also agrees, I'll apply it to branch 1.1 of CVS. -- Nicolas Baradakis
Nicolas Baradakis wrote:
Many thanks for the second patch, and thanks for doing the testing, too. Your efforts are lowering the amount of work on our side to a minimum.
Yes. It's much appreciated.
I've reviewed your last patch and it looks ok to me. If Alan also agrees, I'll apply it to branch 1.1 of CVS.
Looks good to me. We should release 1.1.8 soon. Also, if people can test the detail file handling in CVS head, that would help a lot, too. It's been completely re-written, and tested on my systems. But... Alan DeKok.
participants (3)
-
Alan DeKok -
Brian De Wolf -
Nicolas Baradakis