Re: [Libevent-users] EV_CHANGE_xxx macros

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Mon, Oct 25, 2010 at 4:56 PM, Gilad Benjamini
<gilad@xxxxxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.