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
On 1 March 2016 at 08:43, Johannes Berg <[email protected]> wrote:
> 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 ...
>
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
Hello Johannes,
On 1 March 2016 at 11:15, João Paulo Rechi Vita <[email protected]> wrote:
> On 1 March 2016 at 08:43, Johannes Berg <[email protected]> wrote:
>>
>> 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.
>
I just saw you picked this patch with Jouni's fix, thanks!
--
João Paulo Rechi Vita
http://about.me/jprvita