apmd/apm: propagate failure to apm, make apm report failure

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

apmd/apm: propagate failure to apm, make apm report failure

Klemens Nanni-2
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?

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 26 Mar 2021 21:21:55 -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 26 Mar 2021 21:21:55 -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
@@ -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 26 Mar 2021 21:21:55 -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,50 @@ 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)
+ sleep(0);
+ if ((ret = ioctl(ctl_fd, APM_IOC_SUSPEND, 0)) == -1)
  logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
+ 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 +529,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)
+ logmsg(LOG_WARNING, "%s: %s", apm_state(state), strerror(errno));
+ else
+ 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 26 Mar 2021 21:21:55 -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";
+}
+}

Reply | Threaded
Open this post in threaded view
|

Re: apmd/apm: propagate failure to apm, make apm report failure

Klemens Nanni-2
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.

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 26 Mar 2021 21:48: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 26 Mar 2021 21:48:36 -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
@@ -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 26 Mar 2021 21:48: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));
+ 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)
+ logmsg(LOG_WARNING, "%s: %s", apm_state(state), strerror(errno));
+ else
+ 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 26 Mar 2021 21:48:37 -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";
+}
+}

Reply | Threaded
Open this post in threaded view
|

Re: apmd/apm: propagate failure to apm, make apm report failure

Klemens Nanni-2
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.


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
@@ -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));
+ 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)
+ logmsg(LOG_WARNING, "%s: %s", apm_state(state), strerror(errno));
+ else
+ 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";
+}
+}

Reply | Threaded
Open this post in threaded view
|

Re: apmd/apm: propagate failure to apm, make apm report failure

Dave Voutila-2
In reply to this post by Klemens Nanni-2

Klemens Nanni writes:

> 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.
>>

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.

> Correct diff without fat fingered sleep(3) this time.
>
> 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 26 Mar 2021 21:48: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 26 Mar 2021 21:48:36 -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
> @@ -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 26 Mar 2021 21:48: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;

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);
>   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.

> + 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)
> + logmsg(LOG_WARNING, "%s: %s", apm_state(state), strerror(errno));
> + else
> + 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 26 Mar 2021 21:48:37 -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";
> +}
> +}

Reply | Threaded
Open this post in threaded view
|

Re: apmd/apm: propagate failure to apm, make apm report failure

Klemens Nanni-2
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.

Reply | Threaded
Open this post in threaded view
|

Re: apmd/apm: propagate failure to apm, make apm report failure

Jeremie Courreges-Anglas-2
In reply to this post by Klemens Nanni-2
On Fri, Apr 02 2021, Klemens Nanni <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: apmd/apm: propagate failure to apm, make apm report failure

Klemens Nanni-2
On Tue, Apr 06, 2021 at 11:35:44PM +0200, Jeremie Courreges-Anglas wrote:
> On Fri, Apr 02 2021, Klemens Nanni <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: apmd/apm: propagate failure to apm, make apm report failure

Jeremie Courreges-Anglas-2
On Wed, Apr 07 2021, Klemens Nanni <[hidden email]> wrote:
> On Tue, Apr 06, 2021 at 11:35:44PM +0200, Jeremie Courreges-Anglas wrote:

[...]

>> > @@ -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.

And those functions could return zero/errno, this saves a few more
lines.  ok?

BTW now handle_client() could stop setting newstate on error.  But as far as
I'm concerned that's for another day and another diff.



Index: apmd.c
===================================================================
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.104
diff -u -p -r1.104 apmd.c
--- apmd.c 6 Apr 2021 22:12:48 -0000 1.104
+++ apmd.c 6 Apr 2021 22:27:26 -0000
@@ -271,18 +271,15 @@ handle_client(int sock_fd, int ctl_fd)
  switch (cmd.action) {
  case SUSPEND:
  reply.newstate = SUSPENDING;
- if (suspend(ctl_fd) == -1)
- reply.error = errno;
+ reply.error = suspend(ctl_fd);
  break;
  case STANDBY:
  reply.newstate = STANDING_BY;
- if (stand_by(ctl_fd) == -1)
- reply.error = errno;
+ reply.error = stand_by(ctl_fd);
  break;
  case HIBERNATE:
  reply.newstate = HIBERNATING;
- if (hibernate(ctl_fd) == -1)
- reply.error = errno;
+ reply.error = hibernate(ctl_fd);
  break;
  case SETPERF_LOW:
  reply.newstate = NORMAL;
@@ -329,46 +326,58 @@ handle_client(int sock_fd, int ctl_fd)
 int
 suspend(int ctl_fd)
 {
- int ret;
+ int error = 0;
 
  logmsg(LOG_NOTICE, "system suspending");
  power_status(ctl_fd, 1, NULL);
  do_etc_file(_PATH_APM_ETC_SUSPEND);
  sync();
  sleep(1);
- if ((ret = ioctl(ctl_fd, APM_IOC_SUSPEND, 0)) == -1)
+
+ if (ioctl(ctl_fd, APM_IOC_SUSPEND, 0) == -1) {
+ error = errno;
  logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
- return (ret);
+ }
+
+ return error;
 }
 
 int
 stand_by(int ctl_fd)
 {
- int ret;
+ int error = 0;
 
  logmsg(LOG_NOTICE, "system entering standby");
  power_status(ctl_fd, 1, NULL);
  do_etc_file(_PATH_APM_ETC_STANDBY);
  sync();
  sleep(1);
- if ((ret = ioctl(ctl_fd, APM_IOC_STANDBY, 0)) == -1)
+
+ if (ioctl(ctl_fd, APM_IOC_STANDBY, 0) == -1) {
+ error = errno;
  logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
- return (ret);
+ }
+
+ return error;
 }
 
 int
 hibernate(int ctl_fd)
 {
- int ret;
+ int error = 0;
 
  logmsg(LOG_NOTICE, "system hibernating");
  power_status(ctl_fd, 1, NULL);
  do_etc_file(_PATH_APM_ETC_HIBERNATE);
  sync();
  sleep(1);
- if ((ret = ioctl(ctl_fd, APM_IOC_HIBERNATE, 0)) == -1)
+
+ if (ioctl(ctl_fd, APM_IOC_HIBERNATE, 0) == -1) {
+ error = errno;
  logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
- return (ret);
+ }
+
+ return error;
 }
 
 void


--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: apmd/apm: propagate failure to apm, make apm report failure

Klemens Nanni-2
On Wed, Apr 07, 2021 at 12:35:12AM +0200, Jeremie Courreges-Anglas wrote:
> And those functions could return zero/errno, this saves a few more
> lines.  ok?
Even better, OK kn