Messages in this thread Patch in this message | | | From | João Paulo Rechi Vita <> | Date | Tue, 9 Feb 2016 16:35:19 -0500 | Subject | Re: [PATCH 8/9] rfkill: Userspace control for airplane mode |
| |
On 8 February 2016 at 17:53, Julian Calaby <julian.calaby@gmail.com> wrote:
>> + if (ev.op == RFKILL_OP_AIRPLANE_MODE_RELEASE) { >> + if (rfkill_apm_owned && !data->is_apm_owner) { > > Are you sure this is correct? > > In the case that airplane mode isn't owned, the > rfkill_apm_led_trigger_event() call will be a noop, so we should > arguably not be calling it. >
Ok, I'm changing the check to be consistent with _CHANGE, so the call only succeeds if (rfkill_apm_owned && data->is_apm_owner), and return an error otherwise.
> Also, should we just fail silently if we're not the owner? I.e. what > does userspace learn from this op failing and is that useful? >
I think it is better to return an error every time userspace is trying to call an operation that it was not supposed to call at a certain state (without acquiring control of the airplane-mode indicator). If a program has a logic error that makes it call _RELEASE without calling _ACQUIRE first, it's easier for the programmer to spot the problem if we return an error here.
>> + count = -EACCES; >> + } else { >> + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur; >> + >> + rfkill_apm_owned = false; >> + data->is_apm_owner = false; >> + rfkill_apm_led_trigger_event(state); >> + } >> + } >> + >> + if (ev.op == RFKILL_OP_AIRPLANE_MODE_CHANGE) { >> + if (rfkill_apm_owned && data->is_apm_owner) >> + rfkill_apm_led_trigger_event(ev.soft); >> + else >> + count = -EACCES; >> + } >> + >> if (ev.op == RFKILL_OP_CHANGE_ALL) >> rfkill_update_global_state(ev.type, ev.soft); >> >> @@ -1230,7 +1261,17 @@ static int rfkill_fop_release(struct inode *inode, struct file *file) >> struct rfkill_int_event *ev, *tmp; >> >> mutex_lock(&rfkill_global_mutex); >> + >> + if (data->is_apm_owner) { >> + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur; >> + >> + rfkill_apm_owned = false; >> + data->is_apm_owner = false; >> + rfkill_apm_led_trigger_event(state); > > Also, this code is duplicated from the _RELEASE op above. Would it > make sense to factor it out into a separate function? >
Yes, makes sense. This also made me notice I was assigning a negative value to a size_t variable (count).
>> + } >> + >> list_del(&data->list); >> + > > (extra line) >
After factoring out the _RELEASE code it looks better without this additional line.
>> mutex_unlock(&rfkill_global_mutex); >> >> mutex_destroy(&data->mtx); > > Thanks, >
Thanks for the review, Julian. I'm sending an updated version.
-- João Paulo Rechi Vita http://about.me/jprvita
| |