Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
rfkill: Delay writes until exit (#5768) #5815
Conversation
| + int state; | ||
| +} write_queue_item; | ||
| + | ||
| +static LIST_HEAD(write_queue_item, write_queue); |
poettering
Aug 31, 2017
Owner
no, unnecessary global variables please. Doesn't matter much, but I think it would be nicer to avoid this, and just make it a local varibale of main() or so, which is passed through as pointer... dunno.
| + | ||
| +static LIST_HEAD(write_queue_item, write_queue); | ||
| + | ||
| + |
| - return r; | ||
| - | ||
| - r = determine_state_file(udev, event, device, &state_file); | ||
| + r = determine_state_file(udev, event, &state_file); |
poettering
Aug 31, 2017
Owner
maybe split out this little rearrangement in a separete commit in this PR? (doesn't matter though, it's fine this way, too)
| + struct write_queue_item *item, *tmp; | ||
| + | ||
| + LIST_FOREACH_SAFE(queue, item, tmp, write_queue) { | ||
| + if ((state_file && strcmp(item->file, state_file) == 0) || idx == item->rfkill_idx) { |
poettering
Aug 31, 2017
Owner
nitpick: we prefer streq() over strcmp() == 0 in systemd sources, see CODING_STYLE.
| + log_debug("Canceled previous save state of '%s' to %s.", one_zero(item->state), item->file); | ||
| + LIST_REMOVE(queue, write_queue, item); | ||
| + free(item->file); | ||
| + mfree(item); |
poettering
Aug 31, 2017
Owner
no need to use mfree() if you don't care about its return value... the whole reason mfree() exists, is so that you can write:
p = mfree(p);and that frees p and also assigns NULL to it...
| + r = write_string_file(item->file, one_zero(item->state), WRITE_STRING_FILE_CREATE|WRITE_STRING_FILE_ATOMIC); | ||
| + if (r < 0) { | ||
| + result = r; | ||
| + log_error_errno(r, "Failed to write state file %s: %m", item->file); |
poettering
Aug 31, 2017
Owner
if you continue the loop on error, then the error message should be downgraded to warning i figure...
| + log_error_errno(r, "Failed to write state file %s: %m", item->file); | ||
| + } | ||
| + | ||
| + log_debug("Saved state '%s' to %s.", one_zero(item->state), item->file); |
poettering
Aug 31, 2017
Owner
this log line should probably only be shown, if the if block above doesn't apply.. i.e. you probably want an else line there
| + log_debug("Saved state '%s' to %s.", one_zero(item->state), item->file); | ||
| + LIST_REMOVE(queue, write_queue, item); | ||
| + free(item->file); | ||
| + mfree(item); |
|
And I am very sorry for not looking into this any earlier! Somehow it escaped me for too long. Sorry! |
poettering
added
reviewed/needs-rework
rfkill
labels
Aug 31, 2017
|
Whoops, should have tested before pushing and not after (passed the list head in the wrong manner at first). I think I have addressed everything you mentioned. Thanks for the review. I actually kind of forgot to ping you myself. |
| + log_debug("Canceled previous save state of '%s' to %s.", one_zero(item->state), item->file); | ||
| + LIST_REMOVE(queue, *write_queue, item); | ||
| + free(item->file); | ||
| + free(item); |
poettering
Aug 31, 2017
Owner
sorry I missed this on the first review, but I think we should probably have a destructor of some kind for this, i.e. write_queue_item_unlink() or so, that does the three steps above... after all, we have the very same three lines further down again, and we can share that then, and make it more readable
| + error_logged = true; | ||
| + } else { | ||
| + log_warning_errno(r, "Failed to write state file %s: %m", item->file); | ||
| + } |
| + } | ||
| + } else { | ||
| + log_debug("Saved state '%s' to %s.", one_zero(item->state), item->file); | ||
| + } |
poettering
removed
the
reviewed/needs-rework
label
Sep 4, 2017
poettering
merged commit d398cf4
into
systemd:master
Sep 4, 2017
|
Thanks! |
benzea commentedApr 26, 2017
On thinkpads there are two rfkill devices for bluetooth. The first is an
ACPI switch which powers down the USB dongle and the second one is the
USB dongle itself. So when userspace decides to enable rfkill on all
devices systemd would randomly save the soft block state of the USB
dongle. This later causes issue when re-enabling the devie as
systemd-rfkill would put the USB dongle into soft block state right
after the ACPI rfkill switch is unblocked by userspace.
The simple way to avoid this is to not store rfkill changes for devices
that disappear shortly after. That way only the "main" ACPI switch will
get stored and systemd-rfkill will not end up blocking the device right
after it is being added back again.