On Mon, Mar 18, 2013 at 11:15:55AM +0000, Stuart Henderson wrote:
> Retrying EBUSY definitely makes sense to me.
>
> Wonder if it's worth having a fixed minimum (i.e. "timer = 500 +
> arc4random_uniform(10000)" or something)?
>
> I'm undecided about s/log_warn/fatalx/, in some ways it's better
> if it keeps operating, in other ways it's better to fail...
>
> Big problem with the diff is that it changes handling for the other
> ways in which DIOCXCOMMIT can fail (ENODEV, EFAULT, EINVAL). They are
> no longer treated by relayd as a failure.
>
> Also indentation nit in the continuation line of the new "if (ioctl...)".
>
Diff with your suggestions applied, about the s/log_warn/fatal I am undecided
too.
Cheers & Thanks
Giovanni
Index: pfe_filter.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/pfe_filter.c,v
retrieving revision 1.52
diff -u -p -r1.52 pfe_filter.c
--- pfe_filter.c 19 Oct 2012 16:49:50 -0000 1.52
+++ pfe_filter.c 18 Mar 2013 11:27:59 -0000
@@ -354,9 +354,24 @@ transaction_init(struct relayd *env, con
int
transaction_commit(struct relayd *env)
{
+int timer = 0;
+
if (ioctl(env->sc_pf->dev, DIOCXCOMMIT,
- &env->sc_pf->pft) == -1)
+ &env->sc_pf->pft) == -1) {
+ /*
+ * if DIOCXCOMMIT fails with EBUSY, retry after some
milliseconds
+ */
+ if (errno == EBUSY) {
+ timer = 500 + arc4random_uniform(10000);
+ usleep(timer);
+ if (ioctl(env->sc_pf->dev, DIOCXCOMMIT,
+ &env->sc_pf->pft) == -1)
+ return (-1);
+ else
+ return (0);
+ }
return (-1);
+ }
return (0);
}
@@ -509,7 +524,7 @@ sync_ruleset(struct relayd *env, struct
log_debug("%s: rule added to anchor \"%s\"", __func__, anchor);
}
if (transaction_commit(env) == -1)
- log_warn("%s: add rules transaction failed", __func__);
+ fatalx("add rules transaction failed");
return;
toolong:
