On Tue, Apr 06, 2021 at 11:35:44PM +0200, Jeremie Courreges-Anglas wrote:
> On Fri, Apr 02 2021, Klemens Nanni <[email protected]> wrote:
> > @@ -64,6 +64,7 @@ struct apm_reply {
> > enum apm_perfmode perfmode;
> > int cpuspeed;
> > struct apm_power_info batterystate;
> > + int error;
> > };
> >
> > #define APMD_VNO 3
>
> You're changing the size/layout of the structure, so this should be
> bumped to avoid weird behavior where apm and apmd are out of sync.
Oops. I've always rebuilt both apmd and apm and did not test the case
of them being out of sync -- did so now and that should obviously be
bumped, thanks.
> > @@ -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));
>
> If you hit the error case logmsg() might clobber the errno and make it
> unavailable later...
Right, better use a "save_errno" in all three functions, then.
> > @@ -512,20 +528,12 @@ main(int argc, char *argv[])
> > break;
> >
> > if (rv == 1 && ev->ident == sock_fd) {
> > - switch (handle_client(sock_fd, ctl_fd)) {
> > - case NORMAL:
> > - break;
> > - case SUSPENDING:
> > - suspend(ctl_fd);
> > - break;
> > - case STANDING_BY:
> > - stand_by(ctl_fd);
> > - break;
> > - case HIBERNATING:
> > - hibernate(ctl_fd);
> > - break;
> > - }
> > - continue;
> > + int state;
> > +
> > + if ((state = handle_client(sock_fd, ctl_fd)) == -1)
>
> handle_client returns an enum apm_state, not -1, so the error case can't
> happen. Since the caller of handle_client() doesn't use the return
> value any more, better simplify the code.
>
> > + logmsg(LOG_WARNING, "%s: %s", apm_state(state),
> > strerror(errno));
>
> See the comment about errno being possibly unusable here. I don't think
> we need two error lines in the logs so I'd drop this one.
>
> > + else
>
> This else isn't correct. If an error was possible we should restart the
> loop.
Good catch, I agree with you in all three points.
> > + continue;
> > }
> >
> > suspends = standbys = hibernates = resumes = 0;
> > Index: usr.sbin/apmd/apmsubr.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/apmd/apmsubr.c,v
> > retrieving revision 1.9
> > diff -u -p -r1.9 apmsubr.c
> > --- usr.sbin/apmd/apmsubr.c 23 Sep 2020 05:50:26 -0000 1.9
> > +++ usr.sbin/apmd/apmsubr.c 2 Apr 2021 16:07:41 -0000
> > @@ -83,3 +83,20 @@ perf_mode(int mode)
> > return "invalid";
> > }
> > }
> > +
> > +const char *
> > +apm_state(int apm_state)
> > +{
> > + switch (apm_state) {
> > + case NORMAL:
> > + return "normal";
> > + case SUSPENDING:
> > + return "suspend";
> > + case STANDING_BY:
> > + return "standby";
> > + case HIBERNATING:
> > + return "hibenate";
> > + default:
> > + return "unknown";
> > +}
>
> Missing indentation.
>
> > +}
>
> The diff below addresses all the points above. The apm-proto.h change
> would go in a seperate commit. ok?
Thank you, OK kn.
