Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logind: fix getting property OnExternalPower via D-Bus #24976

Merged
merged 1 commit into from Oct 12, 2022

Conversation

mbiebl
Copy link
Contributor

@mbiebl mbiebl commented Oct 12, 2022

The BUS_DEFINE_PROPERTY_GET_GLOBAL macro requires a value as third argument, so we need to call manager_is_on_external_power(). Otherwise the function pointer is interpreted as a boolean and always returns true:

$ busctl get-property org.freedesktop.login1 /org/freedesktop/login1 org.freedesktop.login1.Manager OnExternalPower
b true
$ /lib/systemd/systemd-ac-power  --verbose
no

Thanks: Helmut Grohne
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1021644

The BUS_DEFINE_PROPERTY_GET_GLOBAL macro requires a value as third
argument, so we need to call manager_is_on_external_power(). Otherwise
the function pointer is interpreted as a boolean and always returns
true:

```
$ busctl get-property org.freedesktop.login1 /org/freedesktop/login1 org.freedesktop.login1.Manager OnExternalPower
b true
$ /lib/systemd/systemd-ac-power  --verbose
no
```

Thanks: Helmut Grohne <helmut@subdivi.de>
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1021644
@mbiebl
Copy link
Contributor Author

mbiebl commented Oct 12, 2022

Seems like an obvious backport candidate, so adding the corresponding label

@mbiebl
Copy link
Contributor Author

mbiebl commented Oct 12, 2022

From src/shared/bus-get-properties.h

#define BUS_DEFINE_PROPERTY_GET_GLOBAL(function, bus_type, val) 

#define BUS_DEFINE_PROPERTY_GET(function, bus_type, data_type, get1)

and src/login/logind-dbus.c

static BUS_DEFINE_PROPERTY_GET(property_get_docked, "b", Manager, manager_is_docked_or_external_displays);
static BUS_DEFINE_PROPERTY_GET(property_get_lid_closed, "b", Manager, manager_is_lid_closed);
static BUS_DEFINE_PROPERTY_GET_GLOBAL(property_get_on_external_power, "b", manager_is_on_external_power);

This inconsistency in the API is a bit of a pitfall, so a mistake like this one was bound to happen.

@yuwata yuwata added the login label Oct 12, 2022
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch. LGTM.

@yuwata yuwata added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Oct 12, 2022
@mbiebl mbiebl merged commit 63238ef into systemd:main Oct 12, 2022
@keszybz keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Oct 24, 2022
@keszybz
Copy link
Member

keszybz commented Nov 3, 2022

Already in v251.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants