On 1 March 2016 at 08:43, Johannes Berg [off-list ref] wrote:

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

quoted

quoted

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 ... :)

quoted

quoted

 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).

quoted

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 ...

Yep, that's an important point (not breaking the API/ABI).

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?

I think this patch should keep the original logic, as this is supposed
to be a refactor only. If we decide to remove the check, we should to
it in a separate patch, to make it clear for someone looking at the
history later.

I'm fine with removing the type check (in a separate patch), but I
don't see much gain in doing so.

--
João Paulo Rechi Vita
http://about.me/jprvita