On Mon, Oct 25, 2010 at 4:56 PM, Gilad Benjamini
<gi...@altornetworks.com> wrote:
> The definition of the EV_CHANGE_xxx macros partially relies on the
> definition of a different set of macros (EV_SIGNAL, EV_PERSIST, etc.).
I'd have no objection to making the EV_CHANGE_xxx macros more
independent from the EV_* macros in 2.1. In particular, having
EV_CHANGE_ADD and EV_CHANGE_DEL not overlap with any used bits in EV_*
would be grand.
The point of having EV_CHANGELIST_{ET, PERSIST, SIGNAL} be the same as
EV_{ET,PERSIST,SIGNAL} was so that the code you mention below could
work without needing to do bit-by-bit reassignment of the "if (events
& EV_x) change->read_change |= EV_x" variety. Still, this should be
documented.
Patches welcome on the tracker as always, if somebody wants to work on this. ;)
> There are two issues I see with that.
>
> First, both EV_CHANGE_ADD and EV_TIMEOUT are defined as 1, which might lead
> to situations where a bit can be interpreted as two different things., e.g.
> following this code in event_changelist_add
> if (events & (EV_READ|EV_SIGNAL)) {
> change->read_change = EV_CHANGE_ADD |
> (events & (EV_ET|EV_PERSIST|EV_SIGNAL));
> }
> if (events & EV_WRITE) {
> change->write_change = EV_CHANGE_ADD |
> (events & (EV_ET|EV_PERSIST|EV_SIGNAL));
> }
>
> I haven't actually found any buggy scenario, but it is a bit fishy.
>
> Second, the definitions combine constant values (e.g. 0x02) with macro usage
> (e.g. EV_SIGNAL). This style easily allows human errors where two of these
> macros will end up with the same value.
>
> I don't know the code well enough to suggest a patch so I'll have to settle
> for raising the issue.
yrs,
--
Nick
***********************************************************************
To unsubscribe, send an e-mail to majord...@freehaven.net with
unsubscribe libevent-users in the body.
