On Fri, Apr 02 2021, Klemens Nanni <[email protected]> wrote:
> On Fri, Mar 26, 2021 at 10:49:53PM +0100, Klemens Nanni wrote:
>> On Fri, Mar 26, 2021 at 10:38:30PM +0100, Klemens Nanni wrote:
>> > apm(8) never knows gets the result of the requested power action carried
>> > out by apmd(8), so platforms without suspend/resume support behave like
>> > this:
>> >
>> > $ zzz; echo $?
>> > Suspending system...
>> > 0
>> > $ apm -z; echo $?
>> > System will enter suspend mode momentarily.
>> > 0
>> >
>> > Prior to the latest apmd commit there wasn't even a syslog message
>> > reporting any failure whatsoever.
>> >
>> >
>> > This diff adds an extra `int error' field to `struct apm_reply' which
>> > such that apmd has means to tell apm if something went in simple
>> > errno(2) fashion.
>> >
>> > To do so I need to hoist the power action inside handle_client(),
>> > obviously before the reply it sent back to apm -- currently apmd replies
>> > *before* carrying out the requested power actions.
>> >
>> >
>> > On arm64 it looks like this then:
>> >
>> > $ zzz; echo $?
>> > Suspending system...
>> > zzz: suspend: Operation not supported
>> > 1
>> > $ apm -z; echo $?
>> > System will enter suspend mode momentarily.
>> > apm: suspend: Operation not supported
>> > 1
>> >
>> > amd64 notebooks for example keep suspending without error messages and
>> > exit zero as before, i.e. expected behaviour stays unchanged for
>> > platforms that already work.
>> >
>> > Feedback? OK?
>> Correct diff without fat fingered sleep(3) this time.
> Anyone interested in this attempt to improve user experience on
> platforms without proper power management support?
>
> suspend/resume is just a special case; in general I'm fond of programs
> telling me when things like ioctls go wrong.
Sorry for being late to the party. I agree with the direction but
I have some changes on top.
> Index: usr.sbin/apm/apm.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/apm/apm.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 apm.c
> --- usr.sbin/apm/apm.c 23 Sep 2020 05:50:26 -0000 1.37
> +++ usr.sbin/apm/apm.c 2 Apr 2021 16:07:36 -0000
> @@ -97,6 +97,7 @@ do_zzz(int fd, enum apm_action action)
> struct apm_command command;
> struct apm_reply reply;
> char *msg;
> + int ret;
>
> switch (action) {
> case NONE:
> @@ -117,7 +118,10 @@ do_zzz(int fd, enum apm_action action)
> }
>
> printf("%s...\n", msg);
> - exit(send_command(fd, &command, &reply));
> + ret = send_command(fd, &command, &reply);
> + if (reply.error)
> + errx(1, "%s: %s", apm_state(reply.newstate),
> strerror(reply.error));
> + exit(ret);
> }
>
> static int
> @@ -418,5 +422,7 @@ balony:
> default:
> break;
> }
> + if (reply.error)
> + errx(1, "%s: %s", apm_state(reply.newstate),
> strerror(reply.error));
> return (0);
> }
> Index: usr.sbin/apmd/apm-proto.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/apmd/apm-proto.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 apm-proto.h
> --- usr.sbin/apmd/apm-proto.h 23 Sep 2020 05:50:26 -0000 1.10
> +++ usr.sbin/apmd/apm-proto.h 2 Apr 2021 16:07:37 -0000
> @@ -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.
> @@ -71,3 +72,4 @@ struct apm_reply {
> extern const char *battstate(int state);
> extern const char *ac_state(int state);
> extern const char *perf_mode(int mode);
> +extern const char *apm_state(int apm_state);
> Index: usr.sbin/apmd/apmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 apmd.c
> --- usr.sbin/apmd/apmd.c 25 Mar 2021 20:46:55 -0000 1.102
> +++ usr.sbin/apmd/apmd.c 2 Apr 2021 16:07:37 -0000
> @@ -65,9 +65,9 @@ void usage(void);
> int power_status(int fd, int force, struct apm_power_info *pinfo);
> int bind_socket(const char *sn);
> enum apm_state handle_client(int sock_fd, int ctl_fd);
> -void suspend(int ctl_fd);
> -void stand_by(int ctl_fd);
> -void hibernate(int ctl_fd);
> +int suspend(int ctl_fd);
> +int stand_by(int ctl_fd);
> +int hibernate(int ctl_fd);
> void resumed(int ctl_fd);
> void setperfpolicy(char *policy);
> void sigexit(int signo);
> @@ -266,16 +266,23 @@ handle_client(int sock_fd, int ctl_fd)
> return NORMAL;
> }
>
> + reply.error = 0;
> power_status(ctl_fd, 0, &reply.batterystate);
> 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));
If you hit the error case logmsg() might clobber the errno and make it
unavailable later...
> + return (ret);
> }
>
> -void
> +int
> stand_by(int ctl_fd)
> {
> + int ret;
> +
> logmsg(LOG_NOTICE, "system entering standby");
> power_status(ctl_fd, 1, NULL);
> do_etc_file(_PATH_APM_ETC_STANDBY);
> sync();
> sleep(1);
> - if (ioctl(ctl_fd, APM_IOC_STANDBY, 0) == -1)
> + if ((ret = ioctl(ctl_fd, APM_IOC_STANDBY, 0)) == -1)
> logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
> + return (ret);
> }
>
> -void
> +int
> hibernate(int ctl_fd)
> {
> + int ret;
> +
> logmsg(LOG_NOTICE, "system hibernating");
> power_status(ctl_fd, 1, NULL);
> do_etc_file(_PATH_APM_ETC_HIBERNATE);
> sync();
> sleep(1);
> - if (ioctl(ctl_fd, APM_IOC_HIBERNATE, 0) == -1)
> + if ((ret = ioctl(ctl_fd, APM_IOC_HIBERNATE, 0)) == -1)
> logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
> + return (ret);
> }
>
> void
> @@ -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.
> + 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?
Index: apm-proto.h
===================================================================
RCS file: /d/cvs/src/usr.sbin/apmd/apm-proto.h,v
retrieving revision 1.11
diff -u -p -r1.11 apm-proto.h
--- apm-proto.h 6 Apr 2021 20:30:32 -0000 1.11
+++ apm-proto.h 6 Apr 2021 21:09:59 -0000
@@ -67,7 +67,7 @@ struct apm_reply {
int error;
};
-#define APMD_VNO 3
+#define APMD_VNO 4
extern const char *battstate(int state);
extern const char *ac_state(int state);
Index: apmd.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.103
diff -u -p -r1.103 apmd.c
--- apmd.c 6 Apr 2021 20:30:32 -0000 1.103
+++ apmd.c 6 Apr 2021 21:15:13 -0000
@@ -64,7 +64,7 @@ extern char *__progname;
void usage(void);
int power_status(int fd, int force, struct apm_power_info *pinfo);
int bind_socket(const char *sn);
-enum apm_state handle_client(int sock_fd, int ctl_fd);
+void handle_client(int sock_fd, int ctl_fd);
int suspend(int ctl_fd);
int stand_by(int ctl_fd);
int hibernate(int ctl_fd);
@@ -231,7 +231,7 @@ bind_socket(const char *sockname)
return sock;
}
-enum apm_state
+void
handle_client(int sock_fd, int ctl_fd)
{
/* accept a handle from the client, process it, then clean up */
@@ -251,19 +251,19 @@ handle_client(int sock_fd, int ctl_fd)
cli_fd = accept(sock_fd, (struct sockaddr *)&from, &fromlen);
if (cli_fd == -1) {
logmsg(LOG_INFO, "client accept failure: %s", strerror(errno));
- return NORMAL;
+ return;
}
if (recv(cli_fd, &cmd, sizeof(cmd), 0) != sizeof(cmd)) {
(void) close(cli_fd);
logmsg(LOG_INFO, "client size botch");
- return NORMAL;
+ return;
}
if (cmd.vno != APMD_VNO) {
close(cli_fd); /* terminate client */
/* no error message, just drop it. */
- return NORMAL;
+ return;
}
bzero(&reply, sizeof(reply));
@@ -324,8 +324,6 @@ handle_client(int sock_fd, int ctl_fd)
if (send(cli_fd, &reply, sizeof(reply), 0) != sizeof(reply))
logmsg(LOG_INFO, "reply to client botched");
close(cli_fd);
-
- return reply.newstate;
}
int
@@ -528,12 +526,8 @@ main(int argc, char *argv[])
break;
if (rv == 1 && ev->ident == sock_fd) {
- int state;
-
- if ((state = handle_client(sock_fd, ctl_fd)) == -1)
- logmsg(LOG_WARNING, "%s: %s", apm_state(state),
strerror(errno));
- else
- continue;
+ handle_client(sock_fd, ctl_fd);
+ continue;
}
suspends = standbys = hibernates = resumes = 0;
Index: apmsubr.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/apmd/apmsubr.c,v
retrieving revision 1.10
diff -u -p -r1.10 apmsubr.c
--- apmsubr.c 6 Apr 2021 20:30:32 -0000 1.10
+++ apmsubr.c 6 Apr 2021 21:10:11 -0000
@@ -98,5 +98,5 @@ apm_state(int apm_state)
return "hibenate";
default:
return "unknown";
-}
+ }
}
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
