Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

[Johannes Berg]

On Tue, 2016-03-01 at 00:39 +0200, Jouni Malinen wrote:

> > I agree there is a difference in the logic here,

Gah. I thought I'd reviewed the logic and made sure there's no
difference ... :)

> >  thanks for taking the
> > time to point it out so clearly, and sorry for missing this. But AFAIU
> > userspace should not call RFKILL_OP_CHANGE with ev.type ==
> > RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
> > block/unblock one RFKill switch, and it is not possible to create a
> > RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
> > return NULL).

> Interesting. Maybe Johannes can comment on that part since I think he
> wrote the code that interacts with kernel for the rfkill test cases.

So first of all, it seems that this argument is invalid since we can't break the ABI/API here; although perhaps if it's only a test case ...

Oh. It took me a while, but I see now. The original intent (I think)
was that with RFKILL_OP_CHANGE, the type would be ignored entirely. It
seems that the (my) original intent wouldn't have been to force
userspace to specify *both* the index and the type, but instead do

OP_CHANGE_ALL -> use type (possibly TYPE_ALL, ignoring idx)
OP_CHANGE Â Â -> use idx (ignoring type)


The original code implemented it as follows:

        if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;

-> check the idx only for OP_CHANGE

ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;

-> check the type, allowing _ALL

Now, all userspace that I found sets the ev.type field to TYPE_ALL all
the time; and it had to given these checks.

e.g. from rfkill.py:

# idx, type, op, soft, hard
_event_struct = '@IBBBB'

[...]

  def block(self):
ÂÂÂÂÂÂÂÂrfk = open('/dev/rfkill', 'w')
ÂÂÂÂÂÂÂÂs = struct.pack(_event_struct, self.idx, TYPE_ALL, _OP_CHANGE, 1, 0)
ÂÂÂÂÂÂÂÂrfk.write(s)
ÂÂÂÂÂÂÂÂrfk.close()


This check, originally, probably should've been

        if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL &&
                    ev.op != RFKILL_OP_CHANGE)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;

to ignore the type entirely.

I'm fine with Jouni's change, preserving the original behaviour of
requiring TYPE_ALL or the correct type, but I'm tempted to simply
remove the type check entirely.

Thoughts?

johannes