GNOME Bugzilla – Bug 777508
smartcard: Some fixes for error handling
Last modified: 2017-01-24 09:59:45 UTC
Here are a couple of fixes I made after spotting the following line in my journal: gnome-settings-[641]: g_task_return_error: assertion 'error != NULL' failed I believe they are the only two places in gnome-settings-daemon which could be causing this control flow, so hopefully they fix it. I can’t currently verify because rebuilding and testing g-s-d is a pain and I have limited time. Feel free to reject the patches on that basis.
Created attachment 343842 [details] [review] smartcard: Set an error on cancellation All callers of watch_one_event_from_driver() expect that iff it returns FALSE, an error is set. That was not the case on the cancellation path. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 343843 [details] [review] smartcard: Fix a g_task_return_error(task, NULL) call It’s possible for activate_all_drivers_async_finish() to return FALSE without an error set, in the case that there were no drivers to activate (but there were no errors either). Fix on_all_drivers_activated() to not assume that FALSE means error, thus fixing a warning I saw in my journal: gnome-settings-[641]: g_task_return_error: assertion 'error != NULL' failed Signed-off-by: Philip Withnall <withnall@endlessm.com>
Review of attachment 343842 [details] [review]: Remove the S-o-B please.
Review of attachment 343843 [details] [review]: ::: plugins/smartcard/gsd-smartcard-manager.c @@ +600,3 @@ PK11SlotInfo *login_token; + activate_all_drivers_async_finish (self, result, &error); I'd rather have activate_all_drivers_async_finish() return an error with the code itself saying that there were not drivers to enable.
Created attachment 343941 [details] [review] smartcard: Set an error on cancellation All callers of watch_one_event_from_driver() expect that iff it returns FALSE, an error is set. That was not the case on the cancellation path.
Created attachment 343942 [details] [review] smartcard: Fix a g_task_return_error(task, NULL) call It’s possible for activate_all_drivers_async_finish() to return FALSE without an error set, in the case that there were no drivers to activate (but there were no errors either). Fix it to return a new error code (GSD_SMARTCARD_MANAGER_ERROR_NO_DRIVERS) in that case, to preserve the invariant that a FALSE return value means an error, thus fixing a warning I saw in my journal: gnome-settings-[641]: g_task_return_error: assertion 'error != NULL' failed
(In reply to Bastien Nocera from comment #3) > Review of attachment 343842 [details] [review] [review]: > > Remove the S-o-B please. Out of interest, why drop it? Is it doing any harm? All it means is that I’ve certified I’ve got the rights to the code I’m submitting. If the project doesn’t require that (I don’t know if any GNOME projects do?) it doesn’t matter. (In reply to Bastien Nocera from comment #4) > Review of attachment 343843 [details] [review] [review]: > > ::: plugins/smartcard/gsd-smartcard-manager.c > @@ +600,3 @@ > PK11SlotInfo *login_token; > > + activate_all_drivers_async_finish (self, result, &error); > > I'd rather have activate_all_drivers_async_finish() return an error with the > code itself saying that there were not drivers to enable. Fixed!
Review of attachment 343941 [details] [review]: Yep.
Review of attachment 343942 [details] [review]: ::: plugins/smartcard/gsd-smartcard-manager.c @@ +606,1 @@ + if (error != NULL) { Revert that hunk and: if (g_error_matches(...)) { g_clear_error (&error); } else { g_task_return_error (); return; }
(In reply to Philip Withnall from comment #7) > (In reply to Bastien Nocera from comment #3) > > Review of attachment 343842 [details] [review] [review] [review]: > > > > Remove the S-o-B please. > > Out of interest, why drop it? Is it doing any harm? All it means is that > I’ve certified I’ve got the rights to the code I’m submitting. If the > project doesn’t require that (I don’t know if any GNOME projects do?) it > doesn’t matter. That's what it means *for the kernel*. You can only S-o-b, for the kernel, with a real name as well, which means that we can't use it if we still want anonymous, or pseudonymous contributions. In this case, it doesn't mean anything, and just pollutes the logs.
Review of attachment 343942 [details] [review]: I deliberately left the error reporting in here (rather than squashing the NO_DRIVERS error). I know it’s a change of behaviour, but if I’ve followed the control flow through correctly, I think it just results in a g_debug() entry later on (and nothing else) anyway, so I thought it was best to keep things simple and go with that.
Created attachment 344099 [details] [review] smartcard: Fix a g_task_return_error(task, NULL) call It’s possible for activate_all_drivers_async_finish() to return FALSE without an error set, in the case that there were no drivers to activate (but there were no errors either). Fix it to return a new error code (GSD_SMARTCARD_MANAGER_ERROR_NO_DRIVERS) in that case, to preserve the invariant that a FALSE return value means an error, thus fixing a warning I saw in my journal: gnome-settings-[641]: g_task_return_error: assertion 'error != NULL' failed
Attachment 343941 [details] pushed as fc1f51c - smartcard: Set an error on cancellation Attachment 344099 [details] pushed as b719bc0 - smartcard: Fix a g_task_return_error(task, NULL) call