On Tue, Apr 06, 2021 at 01:44:38PM -0400, Dave Voutila wrote:
> Working on my amd64 system. Sadly I don't have other archs (yet) to
> test this on. But I don't see any reason it should impact any other
> architectures.
>
> >> Feedback? OK?
>
> OK dv@, I do have some feedback/observations below that aren't critical.
Thank you.
> > @@ -266,16 +266,23 @@ handle_client(int sock_fd, int ctl_fd)
> > return NORMAL;
> > }
> >
> > + reply.error = 0;
>
> I noticed the apm_reply struct isn't explicitly zeroed. Given it's
> passed around in whole or in part (.batterystate), is it worth an
> explicit zeroing? (It looks like the client apm.c bzero(3)'s its
> instance before it uses an APM_IOC_GETPOWER ioctl.)
>
> > power_status(ctl_fd, 0, &reply.batterystate);
I don't think that's needed (hasn't been zeroed before and I'm merely
initialising the new field), but using bzero() just like apm(4) does
certainly doesn't hurt and is consistent, so I've committed the diff
whith this on top:
- reply.error = 0;
+ bzero(&reply, sizeof(reply));
> > switch (cmd.action) {
> > case SUSPEND:
> > reply.newstate = SUSPENDING;
> > + if (suspend(ctl_fd) == -1)
> > + reply.error = errno;
> > break;
> > case STANDBY:
> > reply.newstate = STANDING_BY;
> > + if (stand_by(ctl_fd) == -1)
> > + reply.error = errno;
> > break;
> > case HIBERNATE:
> > reply.newstate = HIBERNATING;
> > + if (hibernate(ctl_fd) == -1)
> > + reply.error = errno;
> > break;
> > case SETPERF_LOW:
> > reply.newstate = NORMAL;
> > @@ -321,40 +328,49 @@ handle_client(int sock_fd, int ctl_fd)
> > return reply.newstate;
> > }
> >
> > -void
> > +int
> > suspend(int ctl_fd)
> > {
> > + int ret;
> > +
> > logmsg(LOG_NOTICE, "system suspending");
> > power_status(ctl_fd, 1, NULL);
> > do_etc_file(_PATH_APM_ETC_SUSPEND);
> > sync();
> > sleep(1);
> > - if (ioctl(ctl_fd, APM_IOC_SUSPEND, 0) == -1)
> > + if ((ret = ioctl(ctl_fd, APM_IOC_SUSPEND, 0)) == -1)
> > logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
>
> I wasn't familiar with the apmd(8) stuff until I looked at your diff,
> but if I'm reading correctly (on amd64) the suspend/standby/hibernate
> ioctls effectively execute the apm task async via a kernel thread
> dealing with an acpi task queue.
>
> It looks like there are possibilities for failures (on amd64) that might
> not be caught by the ioctl, but I think that's well beyond a reasonable
> scope for this change.
Yes, this is mainly just to catch the obvious EOPNOTSUPP case.
