Re: sysctl machdep.lidaction=suspend

[Marco Bonetti]

On 07/29, Martin Natano wrote:
> Words are more descriptive than numbers. Let's use them!
> 
> This diff removes
> 
>   sysctl machdep.lidaction=0
>   sysctl machdep.lidaction=1
>   sysctl machdep.lidaction=2
> 
> in favor of the more descriptive
> 
>   sysctl machdep.lidaction=none
>   sysctl machdep.lidaction=suspend
>   sysctl machdep.lidaction=hibernate
> 
> as requested by deraadt.
> 
> Given that there is a diff for poweroff (which I want to see go in)
> floating around on tech@, it might be a good idea to switch now, so
> the numbering doesn't get out of hand.
> 
> Of course this means people will have to update their /etc/sysctl.conf
> _again_. Sorry for that, I missed the opportunity to do both lidaction
> changes in one big swoop.
> 
> Do we want to go there?
> 
> natano
> 
> 
> Index: etc/etc.amd64/sysctl.conf
> ===
> RCS file: /cvs/src/etc/etc.amd64/sysctl.conf,v
> retrieving revision 1.7
> diff -u -p -r1.7 sysctl.conf
> --- etc/etc.amd64/sysctl.conf 2 Mar 2017 10:38:09 -   1.7
> +++ etc/etc.amd64/sysctl.conf 25 Jul 2017 18:40:31 -
> @@ -1,3 +1,3 @@
>  #machdep.allowaperture=2 # See xf86(4)
>  #machdep.kbdreset=1  # permit console CTRL-ALT-DEL to do a nice halt
> -#machdep.lidaction=0 # 1=suspend, 2=hibernate laptop upon lid closing
> +#machdep.lidaction=none  # action upon lid closing: none, 
> suspend or hibernate
> Index: etc/etc.i386/sysctl.conf
> ===
> RCS file: /cvs/src/etc/etc.i386/sysctl.conf,v
> retrieving revision 1.21
> diff -u -p -r1.21 sysctl.conf
> --- etc/etc.i386/sysctl.conf  2 Mar 2017 10:38:09 -   1.21
> +++ etc/etc.i386/sysctl.conf  25 Jul 2017 18:40:35 -
> @@ -1,4 +1,4 @@
>  #machdep.allowaperture=2 # See xf86(4)
>  #machdep.apmhalt=1   # 1=powerdown hack, try if halt -p doesn't work
>  #machdep.kbdreset=1  # permit console CTRL-ALT-DEL to do a nice halt
> -#machdep.lidaction=0 # 1=suspend, 2=hibernate laptop upon lid closing
> +#machdep.lidaction=none  # action upon lid closing: none, 
> suspend or hibernate
> Index: etc/etc.loongson/sysctl.conf
> ===
> RCS file: /cvs/src/etc/etc.loongson/sysctl.conf,v
> retrieving revision 1.4
> diff -u -p -r1.4 sysctl.conf
> --- etc/etc.loongson/sysctl.conf  2 Mar 2017 10:38:09 -   1.4
> +++ etc/etc.loongson/sysctl.conf  25 Jul 2017 18:40:40 -
> @@ -1 +1 @@
> -#machdep.lidaction=0 # 1=suspend, 2=hibernate laptop upon lid closing
> +#machdep.lidaction=none  # action upon lid closing: none, 
> suspend or hibernate
> Index: sys/arch/amd64/amd64/machdep.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.231
> diff -u -p -r1.231 machdep.c
> --- sys/arch/amd64/amd64/machdep.c12 Jul 2017 06:26:32 -  1.231
> +++ sys/arch/amd64/amd64/machdep.c25 Jul 2017 19:37:22 -
> @@ -264,6 +264,8 @@ void  map_tramps(void);
>  void init_x86_64(paddr_t);
>  void (*cpuresetfn)(void);
>  
> +int  sysctl_cpulidaction(void *, size_t *, void *, size_t);
> +
>  #ifdef APERTURE
>  int allowaperture = 0;
>  #endif
> @@ -428,7 +430,7 @@ cpu_sysctl(int *name, u_int namelen, voi
>   extern int amd64_has_xcrypt;
>   dev_t consdev;
>   dev_t dev;
> - int val, error;
> + int error;
>  
>   switch (name[0]) {
>   case CPU_CONSDEV:
> @@ -477,15 +479,7 @@ cpu_sysctl(int *name, u_int namelen, voi
>   case CPU_XCRYPT:
>   return (sysctl_rdint(oldp, oldlenp, newp, amd64_has_xcrypt));
>   case CPU_LIDACTION:
> - val = lid_action;
> - error = sysctl_int(oldp, oldlenp, newp, newlen, );
> - if (!error) {
> - if (val < 0 || val > 2)
> - error = EINVAL;
> - else
> - lid_action = val;
> - }
> - return (error);
> + return (sysctl_cpulidaction(oldp, oldlenp, newp, newlen));
>  #if NPCKBC > 0 && NUKBD > 0
>   case CPU_FORCEUKBD:
>   if (forceukbd)
> @@ -500,6 +494,47 @@ cpu_sysctl(int *name, u_int namelen, voi
>   return (EOPNOTSUPP);
>   }
>   /* NOTREACHED */
> +}
> +
> +int
> +sysctl_cpulidaction(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
> +{
> + char action[10];
> + int err

Re: sysctl machdep.lidaction=suspend

[Jason McIntyre]

On Sat, Jul 29, 2017 at 10:43:55PM +0200, Ingo Schwarze wrote:
> 
> All the same, i'd like to point out that there is a minor documentation
> issue.  Some time ago, we decided that having duplicate descriptions
> of each and every sysctl variable in sysctl(3) and sysctl(8) is bad
> for maintainability, deleted them all from sysctl(8), and made sure
> users of sysctl(8) can understand the full listing in sysctl(3) by
> adding the full string-names used by sysctl(8) to the sysctl(3)
> manual page.
> 
> So after your patch, we would have to document in sysctl(3) that
> CTL_MACHDEP.CPU_LIDACTION = machdep.lidaction takes int on the C
> interface level and strings on the command line, and that
> 0=none, 1=suspend, 2=hibernate.
> 

that would be doable, but...

> While i don't know whether documenting *all* machdep variables in
> sysctl(3) would make sense, machdep.lidaction certainly seems
> important enough to document it.  Same for machdep.kbdreset.
> 

... at present sysctl(3) does not describe any machdep syscts at all. it
never has (or we removed them long ago and i forget). the theory was
that the machdep ctls were a moving target and, by definition, only
relevant to specific platforms. so sysctl(3) didn;t even attempt to
describe them, sysctl(8) listed some of them, and sysctl.conf described
some of them.

maybe there's a bigger discussion possible, but for now it would be
simple, and not inconsistent, to just keep that in the example /etc
file. no immediate doc change would be needed.

jmc

> Yours,
>   Ingo
> 

Re: sysctl machdep.lidaction=suspend

[Ingo Schwarze]

Hi Martin,

Martin Natano wrote on Sat, Jul 29, 2017 at 08:45:05PM +0200:
> On Sat, Jul 29, 2017 at 02:19:50PM +0200, Martin Natano wrote:
>> On Sat, Jul 29, 2017 at 02:03:22PM +0200, Mark Kettenis wrote:

>>> I don't think we want to add string parsing like this in the kernel.
>>> Maybe the sysctl(8) frontend should do the mapping from strings to
>>> numbers?

>> Ok, I'll try to come up with an alternative diff that does the parsing
>> in sysctl(8). Let me fetch my rubber gloves...

> Here's the alternative diff. Ok?

Here are my two cents:  For users, "=hibernate" is easier to
understand than "=2", so i sympathize with sysctl(8) taking
the former.

Users rarely have to use sysctl(3) directly, so i don't think it
matters much whether the C interface takes a string or an int;
do whatever is safer in the kernel.

All the same, i'd like to point out that there is a minor documentation
issue.  Some time ago, we decided that having duplicate descriptions
of each and every sysctl variable in sysctl(3) and sysctl(8) is bad
for maintainability, deleted them all from sysctl(8), and made sure
users of sysctl(8) can understand the full listing in sysctl(3) by
adding the full string-names used by sysctl(8) to the sysctl(3)
manual page.

So after your patch, we would have to document in sysctl(3) that
CTL_MACHDEP.CPU_LIDACTION = machdep.lidaction takes int on the C
interface level and strings on the command line, and that
0=none, 1=suspend, 2=hibernate.

While i don't know whether documenting *all* machdep variables in
sysctl(3) would make sense, machdep.lidaction certainly seems
important enough to document it.  Same for machdep.kbdreset.

Yours,
  Ingo

Re: sysctl machdep.lidaction=suspend

[Martin Natano]

On Sat, Jul 29, 2017 at 02:19:50PM +0200, Martin Natano wrote:
> On Sat, Jul 29, 2017 at 02:03:22PM +0200, Mark Kettenis wrote:
> > 
> > I don't think we want to add string parsing like this in the kernel.
> > Maybe the sysctl(8) frontend should do the mapping from strings to
> > numbers?
> 
> Ok, I'll try to come up with an alternative diff that does the parsing
> in sysctl(8). Let me fetch my rubber gloves...

Here's the alternative diff. Ok?


Index: etc/etc.amd64/sysctl.conf
===
RCS file: /cvs/src/etc/etc.amd64/sysctl.conf,v
retrieving revision 1.7
diff -u -p -r1.7 sysctl.conf
--- etc/etc.amd64/sysctl.conf   2 Mar 2017 10:38:09 -   1.7
+++ etc/etc.amd64/sysctl.conf   25 Jul 2017 18:40:31 -
@@ -1,3 +1,3 @@
 #machdep.allowaperture=2   # See xf86(4)
 #machdep.kbdreset=1# permit console CTRL-ALT-DEL to do a nice halt
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=none# action upon lid closing: none, 
suspend or hibernate
Index: etc/etc.i386/sysctl.conf
===
RCS file: /cvs/src/etc/etc.i386/sysctl.conf,v
retrieving revision 1.21
diff -u -p -r1.21 sysctl.conf
--- etc/etc.i386/sysctl.conf2 Mar 2017 10:38:09 -   1.21
+++ etc/etc.i386/sysctl.conf25 Jul 2017 18:40:35 -
@@ -1,4 +1,4 @@
 #machdep.allowaperture=2   # See xf86(4)
 #machdep.apmhalt=1 # 1=powerdown hack, try if halt -p doesn't work
 #machdep.kbdreset=1# permit console CTRL-ALT-DEL to do a nice halt
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=none# action upon lid closing: none, 
suspend or hibernate
Index: etc/etc.loongson/sysctl.conf
===
RCS file: /cvs/src/etc/etc.loongson/sysctl.conf,v
retrieving revision 1.4
diff -u -p -r1.4 sysctl.conf
--- etc/etc.loongson/sysctl.conf2 Mar 2017 10:38:09 -   1.4
+++ etc/etc.loongson/sysctl.conf25 Jul 2017 18:40:40 -
@@ -1 +1 @@
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=none# action upon lid closing: none, 
suspend or hibernate
Index: sbin/sysctl/sysctl.c
===
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.228
diff -u -p -r1.228 sysctl.c
--- sbin/sysctl/sysctl.c19 Jul 2017 06:30:54 -  1.228
+++ sbin/sysctl/sysctl.c29 Jul 2017 16:57:13 -
@@ -158,6 +158,15 @@ struct list secondlevel[] = {
{ 0, 0 },   /* CTL_VFS */
 };
 
+struct enum_vals {
+   int  size;
+   const char  **names;
+};
+#ifdef CPU_LIDACTION
+const char *cpu_lidaction_names[] = { "none", "suspend", "hibernate" };
+struct enum_vals cpu_lidaction_enum = { 3, cpu_lidaction_names };
+#endif
+
 intAflag, aflag, nflag, qflag;
 
 /*
@@ -167,10 +176,10 @@ int   Aflag, aflag, nflag, qflag;
 #defineBOOTTIME0x0002
 #defineCHRDEV  0x0004
 #defineBLKDEV  0x0008
+#define ENUM   0x0010
 #defineBADDYNAMIC  0x0020
 #defineBIOSGEO 0x0040
 #defineBIOSDEV 0x0080
-#defineMAJ2DEV 0x0100
 #defineUNSIGNED0x0200
 #defineKMEMBUCKETS 0x0400
 #defineLONGARRAY   0x0800
@@ -211,6 +220,8 @@ void print_sensor(struct sensor *);
 int sysctl_chipset(char *, char **, int *, int, int *);
 #endif
 void vfsinit(void);
+int strtoenum(const char *, struct enum_vals *, const char **);
+const char *enumtostr(int, struct enum_vals *);
 
 char *equ = "=";
 
@@ -297,6 +308,7 @@ parse(char *string, int flags)
int indx, type, state, intval, len;
size_t size, newsize = 0;
int lal = 0, special = 0;
+   struct enum_vals *enump;
void *newval = NULL;
int64_t quadval;
struct list *lp;
@@ -615,6 +627,12 @@ parse(char *string, int flags)
if (mib[1] == CPU_CPUFEATURE)
special |= HEX;
 #endif
+#ifdef CPU_LIDACTION
+   if (mib[1] == CPU_LIDACTION) {
+   special |= ENUM;
+   enump = _lidaction_enum;
+   }
+#endif
 #ifdef CPU_BLK2CHR
if (mib[1] == CPU_BLK2CHR) {
if (bufp == NULL)
@@ -700,6 +718,8 @@ parse(char *string, int flags)
case CTLTYPE_INT:
if (special & UNSIGNED)
intval = strtonum(newval, 0, UINT_MAX, );
+   else if (special & ENUM)
+   intval = strtoenum(newval, enump, );
else

Re: sysctl machdep.lidaction=suspend

[Martin Natano]

On Sat, Jul 29, 2017 at 02:03:22PM +0200, Mark Kettenis wrote:
> 
> I don't think we want to add string parsing like this in the kernel.
> Maybe the sysctl(8) frontend should do the mapping from strings to
> numbers?

Ok, I'll try to come up with an alternative diff that does the parsing
in sysctl(8). Let me fetch my rubber gloves...

When the necessary mechanism is there we can also take advantage of it
for setting hw.perfpolicy. That's where I copied the approach from.

> 
> > Index: etc/etc.amd64/sysctl.conf
> > ===
> > RCS file: /cvs/src/etc/etc.amd64/sysctl.conf,v
> > retrieving revision 1.7
> > diff -u -p -r1.7 sysctl.conf
> > --- etc/etc.amd64/sysctl.conf   2 Mar 2017 10:38:09 -   1.7
> > +++ etc/etc.amd64/sysctl.conf   25 Jul 2017 18:40:31 -
> > @@ -1,3 +1,3 @@
> >  #machdep.allowaperture=2   # See xf86(4)
> >  #machdep.kbdreset=1# permit console CTRL-ALT-DEL to do a 
> > nice halt
> > -#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon 
> > lid closing
> > +#machdep.lidaction=none# action upon lid closing: none, 
> > suspend or hibernate
> > Index: etc/etc.i386/sysctl.conf
> > ===
> > RCS file: /cvs/src/etc/etc.i386/sysctl.conf,v
> > retrieving revision 1.21
> > diff -u -p -r1.21 sysctl.conf
> > --- etc/etc.i386/sysctl.conf2 Mar 2017 10:38:09 -   1.21
> > +++ etc/etc.i386/sysctl.conf25 Jul 2017 18:40:35 -
> > @@ -1,4 +1,4 @@
> >  #machdep.allowaperture=2   # See xf86(4)
> >  #machdep.apmhalt=1 # 1=powerdown hack, try if halt -p doesn't work
> >  #machdep.kbdreset=1# permit console CTRL-ALT-DEL to do a 
> > nice halt
> > -#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon 
> > lid closing
> > +#machdep.lidaction=none# action upon lid closing: none, 
> > suspend or hibernate
> > Index: etc/etc.loongson/sysctl.conf
> > ===
> > RCS file: /cvs/src/etc/etc.loongson/sysctl.conf,v
> > retrieving revision 1.4
> > diff -u -p -r1.4 sysctl.conf
> > --- etc/etc.loongson/sysctl.conf2 Mar 2017 10:38:09 -   1.4
> > +++ etc/etc.loongson/sysctl.conf25 Jul 2017 18:40:40 -
> > @@ -1 +1 @@
> > -#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon 
> > lid closing
> > +#machdep.lidaction=none# action upon lid closing: none, 
> > suspend or hibernate
> > Index: sys/arch/amd64/amd64/machdep.c
> > ===
> > RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
> > retrieving revision 1.231
> > diff -u -p -r1.231 machdep.c
> > --- sys/arch/amd64/amd64/machdep.c  12 Jul 2017 06:26:32 -  1.231
> > +++ sys/arch/amd64/amd64/machdep.c  25 Jul 2017 19:37:22 -
> > @@ -264,6 +264,8 @@ voidmap_tramps(void);
> >  void   init_x86_64(paddr_t);
> >  void   (*cpuresetfn)(void);
> >  
> > +intsysctl_cpulidaction(void *, size_t *, void *, size_t);
> > +
> >  #ifdef APERTURE
> >  int allowaperture = 0;
> >  #endif
> > @@ -428,7 +430,7 @@ cpu_sysctl(int *name, u_int namelen, voi
> > extern int amd64_has_xcrypt;
> > dev_t consdev;
> > dev_t dev;
> > -   int val, error;
> > +   int error;
> >  
> > switch (name[0]) {
> > case CPU_CONSDEV:
> > @@ -477,15 +479,7 @@ cpu_sysctl(int *name, u_int namelen, voi
> > case CPU_XCRYPT:
> > return (sysctl_rdint(oldp, oldlenp, newp, amd64_has_xcrypt));
> > case CPU_LIDACTION:
> > -   val = lid_action;
> > -   error = sysctl_int(oldp, oldlenp, newp, newlen, );
> > -   if (!error) {
> > -   if (val < 0 || val > 2)
> > -   error = EINVAL;
> > -   else
> > -   lid_action = val;
> > -   }
> > -   return (error);
> > +   return (sysctl_cpulidaction(oldp, oldlenp, newp, newlen));
> >  #if NPCKBC > 0 && NUKBD > 0
> > case CPU_FORCEUKBD:
> > if (forceukbd)
> > @@ -500,6 +494,47 @@ cpu_sysctl(int *name, u_int namelen, voi
> > return (EOPNOTSUPP);
> > }
> > /* NOTREACHED */
> > +}
> > +
> > +int
> > +sysctl_cpulidaction(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
> > +{
> > +   char action[10];
> > +   int error;
> > +
> > +   switch (lid_action) {
> > +   case 0:
> > +   default:
> > +   strlcpy(action, "none", sizeof(action));
> > +   break;
> > +   case 1:
> > +   strlcpy(action, "suspend", sizeof(action));
> > +   break;
> > +#ifdef HIBERNATE
> > +   case 2:
> > +   strlcpy(action, "hibernate", sizeof(action));
> > +   break;
> > +#endif
> > +   }
> > +
> > +   error = sysctl_string(oldp, oldlenp, newp, newlen, action, 
> > sizeof(action));
> > +   if (error)
> > +  

Re: sysctl machdep.lidaction=suspend

[Mark Kettenis]

> Date: Sat, 29 Jul 2017 13:53:43 +0200
> From: Martin Natano <nat...@natano.net>
> 
> Words are more descriptive than numbers. Let's use them!
> 
> This diff removes
> 
>   sysctl machdep.lidaction=0
>   sysctl machdep.lidaction=1
>   sysctl machdep.lidaction=2
> 
> in favor of the more descriptive
> 
>       sysctl machdep.lidaction=none
>   sysctl machdep.lidaction=suspend
>   sysctl machdep.lidaction=hibernate
> 
> as requested by deraadt.
> 
> Given that there is a diff for poweroff (which I want to see go in)
> floating around on tech@, it might be a good idea to switch now, so
> the numbering doesn't get out of hand.
> 
> Of course this means people will have to update their /etc/sysctl.conf
> _again_. Sorry for that, I missed the opportunity to do both lidaction
> changes in one big swoop.
> 
> Do we want to go there?

I don't think we want to add string parsing like this in the kernel.
Maybe the sysctl(8) frontend should do the mapping from strings to
numbers?

> Index: etc/etc.amd64/sysctl.conf
> ===
> RCS file: /cvs/src/etc/etc.amd64/sysctl.conf,v
> retrieving revision 1.7
> diff -u -p -r1.7 sysctl.conf
> --- etc/etc.amd64/sysctl.conf 2 Mar 2017 10:38:09 -   1.7
> +++ etc/etc.amd64/sysctl.conf 25 Jul 2017 18:40:31 -
> @@ -1,3 +1,3 @@
>  #machdep.allowaperture=2 # See xf86(4)
>  #machdep.kbdreset=1  # permit console CTRL-ALT-DEL to do a nice halt
> -#machdep.lidaction=0 # 1=suspend, 2=hibernate laptop upon lid closing
> +#machdep.lidaction=none  # action upon lid closing: none, 
> suspend or hibernate
> Index: etc/etc.i386/sysctl.conf
> ===
> RCS file: /cvs/src/etc/etc.i386/sysctl.conf,v
> retrieving revision 1.21
> diff -u -p -r1.21 sysctl.conf
> --- etc/etc.i386/sysctl.conf  2 Mar 2017 10:38:09 -   1.21
> +++ etc/etc.i386/sysctl.conf  25 Jul 2017 18:40:35 -
> @@ -1,4 +1,4 @@
>  #machdep.allowaperture=2 # See xf86(4)
>  #machdep.apmhalt=1   # 1=powerdown hack, try if halt -p doesn't work
>  #machdep.kbdreset=1  # permit console CTRL-ALT-DEL to do a nice halt
> -#machdep.lidaction=0 # 1=suspend, 2=hibernate laptop upon lid closing
> +#machdep.lidaction=none  # action upon lid closing: none, 
> suspend or hibernate
> Index: etc/etc.loongson/sysctl.conf
> ===
> RCS file: /cvs/src/etc/etc.loongson/sysctl.conf,v
> retrieving revision 1.4
> diff -u -p -r1.4 sysctl.conf
> --- etc/etc.loongson/sysctl.conf  2 Mar 2017 10:38:09 -   1.4
> +++ etc/etc.loongson/sysctl.conf  25 Jul 2017 18:40:40 -
> @@ -1 +1 @@
> -#machdep.lidaction=0 # 1=suspend, 2=hibernate laptop upon lid closing
> +#machdep.lidaction=none  # action upon lid closing: none, 
> suspend or hibernate
> Index: sys/arch/amd64/amd64/machdep.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.231
> diff -u -p -r1.231 machdep.c
> --- sys/arch/amd64/amd64/machdep.c12 Jul 2017 06:26:32 -  1.231
> +++ sys/arch/amd64/amd64/machdep.c25 Jul 2017 19:37:22 -
> @@ -264,6 +264,8 @@ void  map_tramps(void);
>  void init_x86_64(paddr_t);
>  void (*cpuresetfn)(void);
>  
> +int  sysctl_cpulidaction(void *, size_t *, void *, size_t);
> +
>  #ifdef APERTURE
>  int allowaperture = 0;
>  #endif
> @@ -428,7 +430,7 @@ cpu_sysctl(int *name, u_int namelen, voi
>   extern int amd64_has_xcrypt;
>   dev_t consdev;
>   dev_t dev;
> - int val, error;
> + int error;
>  
>   switch (name[0]) {
>   case CPU_CONSDEV:
> @@ -477,15 +479,7 @@ cpu_sysctl(int *name, u_int namelen, voi
>   case CPU_XCRYPT:
>   return (sysctl_rdint(oldp, oldlenp, newp, amd64_has_xcrypt));
>   case CPU_LIDACTION:
> - val = lid_action;
> - error = sysctl_int(oldp, oldlenp, newp, newlen, );
> - if (!error) {
> - if (val < 0 || val > 2)
> - error = EINVAL;
> - else
> - lid_action = val;
> - }
> - return (error);
> + return (sysctl_cpulidaction(oldp, oldlenp, newp, newlen));
>  #if NPCKBC > 0 && NUKBD > 0
>   case CPU_FORCEUKBD:
>   if (forceukbd)
> @@ -500,6 +494,47 @@ cpu_sysctl(int *name, u_int namelen, voi
>   return (EOPNOTSUPP);
>   }
>

sysctl machdep.lidaction=suspend

[Martin Natano]

Words are more descriptive than numbers. Let's use them!

This diff removes

sysctl machdep.lidaction=0
sysctl machdep.lidaction=1
sysctl machdep.lidaction=2

in favor of the more descriptive

sysctl machdep.lidaction=none
sysctl machdep.lidaction=suspend
sysctl machdep.lidaction=hibernate

as requested by deraadt.

Given that there is a diff for poweroff (which I want to see go in)
floating around on tech@, it might be a good idea to switch now, so
the numbering doesn't get out of hand.

Of course this means people will have to update their /etc/sysctl.conf
_again_. Sorry for that, I missed the opportunity to do both lidaction
changes in one big swoop.

Do we want to go there?

natano


Index: etc/etc.amd64/sysctl.conf
===
RCS file: /cvs/src/etc/etc.amd64/sysctl.conf,v
retrieving revision 1.7
diff -u -p -r1.7 sysctl.conf
--- etc/etc.amd64/sysctl.conf   2 Mar 2017 10:38:09 -   1.7
+++ etc/etc.amd64/sysctl.conf   25 Jul 2017 18:40:31 -
@@ -1,3 +1,3 @@
 #machdep.allowaperture=2   # See xf86(4)
 #machdep.kbdreset=1# permit console CTRL-ALT-DEL to do a nice halt
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=none# action upon lid closing: none, 
suspend or hibernate
Index: etc/etc.i386/sysctl.conf
===
RCS file: /cvs/src/etc/etc.i386/sysctl.conf,v
retrieving revision 1.21
diff -u -p -r1.21 sysctl.conf
--- etc/etc.i386/sysctl.conf2 Mar 2017 10:38:09 -   1.21
+++ etc/etc.i386/sysctl.conf25 Jul 2017 18:40:35 -
@@ -1,4 +1,4 @@
 #machdep.allowaperture=2   # See xf86(4)
 #machdep.apmhalt=1 # 1=powerdown hack, try if halt -p doesn't work
 #machdep.kbdreset=1# permit console CTRL-ALT-DEL to do a nice halt
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=none# action upon lid closing: none, 
suspend or hibernate
Index: etc/etc.loongson/sysctl.conf
===
RCS file: /cvs/src/etc/etc.loongson/sysctl.conf,v
retrieving revision 1.4
diff -u -p -r1.4 sysctl.conf
--- etc/etc.loongson/sysctl.conf2 Mar 2017 10:38:09 -   1.4
+++ etc/etc.loongson/sysctl.conf25 Jul 2017 18:40:40 -
@@ -1 +1 @@
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=none# action upon lid closing: none, 
suspend or hibernate
Index: sys/arch/amd64/amd64/machdep.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.231
diff -u -p -r1.231 machdep.c
--- sys/arch/amd64/amd64/machdep.c  12 Jul 2017 06:26:32 -  1.231
+++ sys/arch/amd64/amd64/machdep.c  25 Jul 2017 19:37:22 -
@@ -264,6 +264,8 @@ voidmap_tramps(void);
 void   init_x86_64(paddr_t);
 void   (*cpuresetfn)(void);
 
+intsysctl_cpulidaction(void *, size_t *, void *, size_t);
+
 #ifdef APERTURE
 int allowaperture = 0;
 #endif
@@ -428,7 +430,7 @@ cpu_sysctl(int *name, u_int namelen, voi
extern int amd64_has_xcrypt;
dev_t consdev;
dev_t dev;
-   int val, error;
+   int error;
 
switch (name[0]) {
case CPU_CONSDEV:
@@ -477,15 +479,7 @@ cpu_sysctl(int *name, u_int namelen, voi
case CPU_XCRYPT:
return (sysctl_rdint(oldp, oldlenp, newp, amd64_has_xcrypt));
case CPU_LIDACTION:
-   val = lid_action;
-   error = sysctl_int(oldp, oldlenp, newp, newlen, );
-   if (!error) {
-   if (val < 0 || val > 2)
-   error = EINVAL;
-   else
-   lid_action = val;
-   }
-   return (error);
+   return (sysctl_cpulidaction(oldp, oldlenp, newp, newlen));
 #if NPCKBC > 0 && NUKBD > 0
case CPU_FORCEUKBD:
if (forceukbd)
@@ -500,6 +494,47 @@ cpu_sysctl(int *name, u_int namelen, voi
return (EOPNOTSUPP);
}
/* NOTREACHED */
+}
+
+int
+sysctl_cpulidaction(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
+{
+   char action[10];
+   int error;
+
+   switch (lid_action) {
+   case 0:
+   default:
+   strlcpy(action, "none", sizeof(action));
+   break;
+   case 1:
+   strlcpy(action, "suspend", sizeof(action));
+   break;
+#ifdef HIBERNATE
+   case 2:
+   strlcpy(action, "hibernate", sizeof(action));
+   break;
+#endif
+   }
+
+   error = sysctl_string(oldp, oldlenp, newp, newlen, action, 
sizeof(action));
+   if (error