Thank you for working on this!

SRU review

> Currently there is an issue with the ac_on_power script where it
thinks that USB-c ports with devices plugged in to them are plugged in
to power.

This isn't a statement of user impact, making it difficult to understand
how changing the stable releases is justified.

But looking through the bug, I found a mention of unattended-upgrades
not running on some systems because they are falsely believing that they
are never on AC power. That implies a user impact of "system isn't
automatically updated as expected". That does justify updating the
stable release.

Is that the only user impact, or are there other cases we need to
consider as well? This is important because if we identify other use
cases then we will be able to know to test them.

> [Test Plan]

Please try to test the actual user impact, which in this case might mean
running unattended-upgrades and ensuring that it does update on such a
system. Running on_ac_power is a helpful diagnostic but it would be
preferable to ensure that nothing else has been missed by testing the
end goal directly as close as is possible.

Further, changing this code could regress existing systems into thinking
they never are on AC power any more and therefore never run unattended-
upgrades any more. This would stop security updates and so the
consequences would be quite severe.

For this reason please further test:

a) Not just one power state of the machine, but that all reasonable
power states of the machine return the expected result...

b) ...across some reasonable set of combinations of machines with and
without USB-C sink and/or source capability...

c) ...considering different "online" states of sinks and sources, given
my review point below and that the current separated code paths suggest
that a bug may be present in this area.

On the actual upload:

1. There are spurious development artifacts being added - various
combinations of {focal,jammy,noble}-powermgmt.debdiff. Please re-upload
with these removed.

2.

> +               power_type="USB*"

This does a glob expansion right here, which I think isn't what is
intended. If there are files in the current directory with this pattern
then they will be expanded here. This needs fixing.

3. I also don't follow the logic used here. What if a sink is found but
it isn't online, and a source exists but is online? Wouldn't that
incorrectly identify the system as being on AC power? As far as I can
tell from just reading the code, the systemd example presented in the MP
doesn't have this behaviour, but the code in this upload would. It's
difficult to tell just from inspection though because the addition of
this code makes it quite convoluted. Am I missing something?

Point 1 above requires re-upload, so I'm rejecting the current items
from the Unapproved queue for now. Please address the other matters
raised above before re-uploading.


** Changed in: powermgmt-base (Ubuntu Focal)
       Status: Confirmed => Incomplete

** Changed in: powermgmt-base (Ubuntu Jammy)
       Status: Confirmed => Incomplete

** Changed in: powermgmt-base (Ubuntu Noble)
       Status: Confirmed => Incomplete

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1980991

Title:
  /usr/sbin/on_ac_power incorrectly reporting ac power status

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/powermgmt-base/+bug/1980991/+subscriptions


-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to