Re: snmpd(8): teach how to fork+exec

2016-10-28 Thread Jeremie Courreges-Anglas
Rafael Zalamena  writes:

> On Sat, Oct 22, 2016 at 10:32:27PM +0200, Rafael Zalamena wrote:
>> On Sat, Oct 22, 2016 at 08:14:16PM +0200, Jeremie Courreges-Anglas wrote:
>> > Rafael Zalamena  writes:
>> > > On Fri, Oct 21, 2016 at 01:26:36PM +0200, Jeremie Courreges-Anglas wrote:
>> > >> Rafael Zalamena  writes:
>> > >> ---snip---
>>
>> Short answer:
>> Yes, you are correct to note this and I think now that it is probably better
>> to write another diff to solve this problem. I'll get back at this diff 
>> later.
>> 
>> ---snip---
>> 
>> Just to clarify:
>> I talked with reyk@ about this global env variables in the last hackathon,
>> and we reached the conclusion that the best way to handle this is to use
>> the ps_env whenever is possible, however since a lot of functions don't
>> get access to ps, we must decide what does less changes to the daemon:
>> 1) Use a single global variable (look at the httpd(8) commits);
>> 2) Keep using the env (relayd(8) case);
>
> The diff that makes snmpd(8) use only one global env is in, now we can
> move on with the fork+exec diff.
>
> Here is the updated diff.
>
> ok?

yup

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



Re: snmpd(8): teach how to fork+exec

2016-10-28 Thread Rafael Zalamena
On Sat, Oct 22, 2016 at 10:32:27PM +0200, Rafael Zalamena wrote:
> On Sat, Oct 22, 2016 at 08:14:16PM +0200, Jeremie Courreges-Anglas wrote:
> > Rafael Zalamena  writes:
> > > On Fri, Oct 21, 2016 at 01:26:36PM +0200, Jeremie Courreges-Anglas wrote:
> > >> Rafael Zalamena  writes:
> > >> ---snip---
>
> Short answer:
> Yes, you are correct to note this and I think now that it is probably better
> to write another diff to solve this problem. I'll get back at this diff later.
> 
> ---snip---
> 
> Just to clarify:
> I talked with reyk@ about this global env variables in the last hackathon,
> and we reached the conclusion that the best way to handle this is to use
> the ps_env whenever is possible, however since a lot of functions don't
> get access to ps, we must decide what does less changes to the daemon:
> 1) Use a single global variable (look at the httpd(8) commits);
> 2) Keep using the env (relayd(8) case);

The diff that makes snmpd(8) use only one global env is in, now we can
move on with the fork+exec diff.

Here is the updated diff.

ok?

Index: proc.c
===
RCS file: /cvs/src/usr.sbin/snmpd/proc.c,v
retrieving revision 1.20
diff -u -p -r1.20 proc.c
--- proc.c  7 Dec 2015 16:05:56 -   1.20
+++ proc.c  28 Oct 2016 08:06:34 -
@@ -1,7 +1,7 @@
 /* $OpenBSD: proc.c,v 1.20 2015/12/07 16:05:56 reyk Exp $  */
 
 /*
- * Copyright (c) 2010 - 2014 Reyk Floeter 
+ * Copyright (c) 2010 - 2016 Reyk Floeter 
  * Copyright (c) 2008 Pierre-Yves Ritschard 
  *
  * Permission to use, copy, modify, and distribute this software for any
@@ -22,6 +22,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -34,8 +35,12 @@
 
 #include "snmpd.h"
 
-voidproc_open(struct privsep *, struct privsep_proc *,
-   struct privsep_proc *, size_t);
+voidproc_exec(struct privsep *, struct privsep_proc *, unsigned int,
+   int, char **);
+voidproc_setup(struct privsep *, struct privsep_proc *, unsigned int);
+voidproc_open(struct privsep *, int, int);
+voidproc_accept(struct privsep *, int, enum privsep_procid,
+   unsigned int);
 voidproc_close(struct privsep *);
 int proc_ispeer(struct privsep_proc *, unsigned int, enum privsep_procid);
 voidproc_shutdown(struct privsep_proc *);
@@ -55,204 +60,383 @@ proc_ispeer(struct privsep_proc *procs, 
return (0);
 }
 
-void
-proc_init(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc)
+enum privsep_procid
+proc_getid(struct privsep_proc *procs, unsigned int nproc,
+const char *proc_name)
 {
-   unsigned int i, j, src, dst;
-   struct privsep_pipes*pp;
+   struct privsep_proc *p;
+   unsigned int proc;
 
-   /*
-* Allocate pipes for all process instances (incl. parent)
-*
-* - ps->ps_pipes: N:M mapping
-* N source processes connected to M destination processes:
-* [src][instances][dst][instances], for example
-* [PROC_RELAY][3][PROC_CA][3]
-*
-* - ps->ps_pp: per-process 1:M part of ps->ps_pipes
-* Each process instance has a destination array of socketpair fds:
-* [dst][instances], for example
-* [PROC_PARENT][0]
-*/
-   for (src = 0; src < PROC_MAX; src++) {
-   /* Allocate destination array for each process */
-   if ((ps->ps_pipes[src] = calloc(ps->ps_ninstances,
-   sizeof(struct privsep_pipes))) == NULL)
-   fatal("proc_init: calloc");
+   for (proc = 0; proc < nproc; proc++) {
+   p = &procs[proc];
+   if (strcmp(p->p_title, proc_name))
+   continue;
 
-   for (i = 0; i < ps->ps_ninstances; i++) {
-   pp = &ps->ps_pipes[src][i];
+   return (p->p_id);
+   }
 
-   for (dst = 0; dst < PROC_MAX; dst++) {
-   /* Allocate maximum fd integers */
-   if ((pp->pp_pipes[dst] =
-   calloc(ps->ps_ninstances,
-   sizeof(int))) == NULL)
-   fatal("proc_init: calloc");
+   return (PROC_MAX);
+}
 
-   /* Mark fd as unused */
-   for (j = 0; j < ps->ps_ninstances; j++)
-   pp->pp_pipes[dst][j] = -1;
-   }
-   }
-   }
+void
+proc_exec(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
+int argc, char **argv)
+{
+   unsigned int proc, nargc, i, proc_i;
+   char**nargv;
+   struct privsep_proc *p;
+   char num[32];
+   int  fd;
+
+   /* Prepare the new process argv. */
+   nargv = calloc(argc + 5, 

Re: snmpd(8): teach how to fork+exec

2016-10-22 Thread Rafael Zalamena
On Sat, Oct 22, 2016 at 08:14:16PM +0200, Jeremie Courreges-Anglas wrote:
> Rafael Zalamena  writes:
> 
> > On Fri, Oct 21, 2016 at 01:26:36PM +0200, Jeremie Courreges-Anglas wrote:
> >> Rafael Zalamena  writes:
> >> > On Fri, Oct 14, 2016 at 06:47:09PM +0200, Rafael Zalamena wrote:
> >> >> On Mon, Sep 26, 2016 at 03:45:59PM +0200, Rafael Zalamena wrote:
> >> >> ---snip---
> >> >
> >> > I got feedback from jca@ that the trap handler wasn't working, so after
> >> > trying to reproduce the problem myself I found one 'env' global variable
> >> > that was not being set and the child process was dying silently.
> >> > (thanks jca@ !)
> >> >
> >> > Instead of depending on snmpe.c:snmpe env initialization (p_init), I'm
> >> > now calling smi_setenv() to do that in the main() function so all 
> >> > children
> >> > get the same behaviour. Also note that we don't have an 'extern' env in
> >> > smi.c anymore.
> >> >
> >> > ok?
> >> 
> >> Works fine here, but then I don't understand the relationship between
> >> static struct snmpd *env in smi.c and struct snmpd *env in snmpe.c.
> >
> > smi.c had a "extern struct snmpd *env" and that variable was only being
> > set during the snmpe initialzation (p_init). Since with fork+exec the
> > child process runs entirely from scratch (no memory / socket sharing with
> > the parent process), we need to set it somewhere else.
> >
> > It is a known problem that everyone that used to set things in the p_init
> > and expected it to work for everyother process was wrong. sunil@ found this
> > the hard-way when he found out that p_env wasn't being set for his process
> > and he noticed that now p_init is ran in the child process already. Before
> > fork+exec the p_init() functions were run by the parent process.
> 
> I understand this, but...
> 
> > To fix the current problem I made the 'env' for smi.c to be a local file
> > global variable and set it in the main() process for every child.
> 
> right now there are mixed uses of a global 'env' variable, a global
> 'snmpd_env' variable, some local 'env' variables set using ps_env or
> cs_env fields.  I fear that throwing another *file-local* 'env' variable
> in the mix makes the code harder to follow.

Short answer:
Yes, you are correct to note this and I think now that it is probably better
to write another diff to solve this problem. I'll get back at this diff later.


Brief background:
With the httpd(8) and relayd(8) we had the same problem: every file depended
on some functions setting a global env and it was being set by some p_init()
that now is not called anymore because of fork+exec.


This case:
Before fork+exec snmpe made the favor to set his global env in p_init which
every file uses and traphandler was also using it indirectly. Now that we
do fork+exec traphandler process don't get env anymore, however traphandler
seems to be only using smi.c, so the new diff addresses this problem.

I agree with you that this is not the final solution, but it is not the
objective of this diff to solve this problem.

If we don't feel that it is safe to proceed with this correction (properly
set env for all files even for traphandler), we must write another diff
that only handles this problem.


Just to clarify:
I talked with reyk@ about this global env variables in the last hackathon,
and we reached the conclusion that the best way to handle this is to use
the ps_env whenever is possible, however since a lot of functions don't
get access to ps, we must decide what does less changes to the daemon:
1) Use a single global variable (look at the httpd(8) commits);
2) Keep using the env (relayd(8) case);

> 
> Also, why would smi.c be special?
> 
> kroute.c:47:extern struct snmpd   *env;
> mib.c:61:extern struct snmpd  *env;
> mps.c:48:extern struct snmpd *env;
> timer.c:45:extern struct snmpd*env;
> trap.c:42:extern struct snmpd *env;
> usm.c:45:extern struct snmpd  *env;



Re: snmpd(8): teach how to fork+exec

2016-10-22 Thread Jeremie Courreges-Anglas
Rafael Zalamena  writes:

> On Fri, Oct 21, 2016 at 01:26:36PM +0200, Jeremie Courreges-Anglas wrote:
>> Rafael Zalamena  writes:
>> > On Fri, Oct 14, 2016 at 06:47:09PM +0200, Rafael Zalamena wrote:
>> >> On Mon, Sep 26, 2016 at 03:45:59PM +0200, Rafael Zalamena wrote:
>> >> ---snip---
>> >
>> > I got feedback from jca@ that the trap handler wasn't working, so after
>> > trying to reproduce the problem myself I found one 'env' global variable
>> > that was not being set and the child process was dying silently.
>> > (thanks jca@ !)
>> >
>> > Instead of depending on snmpe.c:snmpe env initialization (p_init), I'm
>> > now calling smi_setenv() to do that in the main() function so all children
>> > get the same behaviour. Also note that we don't have an 'extern' env in
>> > smi.c anymore.
>> >
>> > ok?
>> 
>> Works fine here, but then I don't understand the relationship between
>> static struct snmpd *env in smi.c and struct snmpd *env in snmpe.c.
>
> smi.c had a "extern struct snmpd *env" and that variable was only being
> set during the snmpe initialzation (p_init). Since with fork+exec the
> child process runs entirely from scratch (no memory / socket sharing with
> the parent process), we need to set it somewhere else.
>
> It is a known problem that everyone that used to set things in the p_init
> and expected it to work for everyother process was wrong. sunil@ found this
> the hard-way when he found out that p_env wasn't being set for his process
> and he noticed that now p_init is ran in the child process already. Before
> fork+exec the p_init() functions were run by the parent process.

I understand this, but...

> To fix the current problem I made the 'env' for smi.c to be a local file
> global variable and set it in the main() process for every child.

right now there are mixed uses of a global 'env' variable, a global
'snmpd_env' variable, some local 'env' variables set using ps_env or
cs_env fields.  I fear that throwing another *file-local* 'env' variable
in the mix makes the code harder to follow.

Also, why would smi.c be special?

kroute.c:47:extern struct snmpd *env;
mib.c:61:extern struct snmpd*env;
mps.c:48:extern struct snmpd *env;
timer.c:45:extern struct snmpd  *env;
trap.c:42:extern struct snmpd   *env;
usm.c:45:extern struct snmpd*env;

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



Re: snmpd(8): teach how to fork+exec

2016-10-21 Thread Rafael Zalamena
On Fri, Oct 21, 2016 at 01:26:36PM +0200, Jeremie Courreges-Anglas wrote:
> Rafael Zalamena  writes:
> > On Fri, Oct 14, 2016 at 06:47:09PM +0200, Rafael Zalamena wrote:
> >> On Mon, Sep 26, 2016 at 03:45:59PM +0200, Rafael Zalamena wrote:
> >> ---snip---
> >
> > I got feedback from jca@ that the trap handler wasn't working, so after
> > trying to reproduce the problem myself I found one 'env' global variable
> > that was not being set and the child process was dying silently.
> > (thanks jca@ !)
> >
> > Instead of depending on snmpe.c:snmpe env initialization (p_init), I'm
> > now calling smi_setenv() to do that in the main() function so all children
> > get the same behaviour. Also note that we don't have an 'extern' env in
> > smi.c anymore.
> >
> > ok?
> 
> Works fine here, but then I don't understand the relationship between
> static struct snmpd *env in smi.c and struct snmpd *env in snmpe.c.

smi.c had a "extern struct snmpd *env" and that variable was only being
set during the snmpe initialzation (p_init). Since with fork+exec the
child process runs entirely from scratch (no memory / socket sharing with
the parent process), we need to set it somewhere else.

It is a known problem that everyone that used to set things in the p_init
and expected it to work for everyother process was wrong. sunil@ found this
the hard-way when he found out that p_env wasn't being set for his process
and he noticed that now p_init is ran in the child process already. Before
fork+exec the p_init() functions were run by the parent process.

To fix the current problem I made the 'env' for smi.c to be a local file
global variable and set it in the main() process for every child.



Re: snmpd(8): teach how to fork+exec

2016-10-21 Thread Jeremie Courreges-Anglas
Rafael Zalamena  writes:

> On Fri, Oct 14, 2016 at 06:47:09PM +0200, Rafael Zalamena wrote:
>> On Mon, Sep 26, 2016 at 03:45:59PM +0200, Rafael Zalamena wrote:
>> > Lets teach snmpd(8) how to fork+exec using the proc.c file from the latest
>> > switchd(8) diff.
>> > 
>> > Note 1: I just tested the basic operations: startup and teardown.
>> > Note 2: the kill with close will be implemented in another diff with the
>> > ps_pid removal.
>> > 
>> 
>> I've update the diff with the latest proc.c changes.
>> 
>> Note: I still have to implement kill with close().
>> 
>
> I got feedback from jca@ that the trap handler wasn't working, so after
> trying to reproduce the problem myself I found one 'env' global variable
> that was not being set and the child process was dying silently.
> (thanks jca@ !)
>
> Instead of depending on snmpe.c:snmpe env initialization (p_init), I'm
> now calling smi_setenv() to do that in the main() function so all children
> get the same behaviour. Also note that we don't have an 'extern' env in
> smi.c anymore.
>
> ok?

Works fine here, but then I don't understand the relationship between
static struct snmpd *env in smi.c and struct snmpd *env in snmpe.c.

> Index: proc.c
> ===
> RCS file: /home/obsdcvs/src/usr.sbin/snmpd/proc.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 proc.c
> --- proc.c7 Dec 2015 16:05:56 -   1.20
> +++ proc.c14 Oct 2016 15:42:19 -
> @@ -1,7 +1,7 @@
>  /*   $OpenBSD: proc.c,v 1.20 2015/12/07 16:05:56 reyk Exp $  */
>  
>  /*
> - * Copyright (c) 2010 - 2014 Reyk Floeter 
> + * Copyright (c) 2010 - 2016 Reyk Floeter 
>   * Copyright (c) 2008 Pierre-Yves Ritschard 
>   *
>   * Permission to use, copy, modify, and distribute this software for any
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -34,8 +35,12 @@
>  
>  #include "snmpd.h"
>  
> -void  proc_open(struct privsep *, struct privsep_proc *,
> - struct privsep_proc *, size_t);
> +void  proc_exec(struct privsep *, struct privsep_proc *, unsigned int,
> + int, char **);
> +void  proc_setup(struct privsep *, struct privsep_proc *, unsigned int);
> +void  proc_open(struct privsep *, int, int);
> +void  proc_accept(struct privsep *, int, enum privsep_procid,
> + unsigned int);
>  void  proc_close(struct privsep *);
>  int   proc_ispeer(struct privsep_proc *, unsigned int, enum privsep_procid);
>  void  proc_shutdown(struct privsep_proc *);
> @@ -55,204 +60,383 @@ proc_ispeer(struct privsep_proc *procs, 
>   return (0);
>  }
>  
> -void
> -proc_init(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc)
> +enum privsep_procid
> +proc_getid(struct privsep_proc *procs, unsigned int nproc,
> +const char *proc_name)
>  {
> - unsigned int i, j, src, dst;
> - struct privsep_pipes*pp;
> + struct privsep_proc *p;
> + unsigned int proc;
>  
> - /*
> -  * Allocate pipes for all process instances (incl. parent)
> -  *
> -  * - ps->ps_pipes: N:M mapping
> -  * N source processes connected to M destination processes:
> -  * [src][instances][dst][instances], for example
> -  * [PROC_RELAY][3][PROC_CA][3]
> -  *
> -  * - ps->ps_pp: per-process 1:M part of ps->ps_pipes
> -  * Each process instance has a destination array of socketpair fds:
> -  * [dst][instances], for example
> -  * [PROC_PARENT][0]
> -  */
> - for (src = 0; src < PROC_MAX; src++) {
> - /* Allocate destination array for each process */
> - if ((ps->ps_pipes[src] = calloc(ps->ps_ninstances,
> - sizeof(struct privsep_pipes))) == NULL)
> - fatal("proc_init: calloc");
> + for (proc = 0; proc < nproc; proc++) {
> + p = &procs[proc];
> + if (strcmp(p->p_title, proc_name))
> + continue;
>  
> - for (i = 0; i < ps->ps_ninstances; i++) {
> - pp = &ps->ps_pipes[src][i];
> + return (p->p_id);
> + }
>  
> - for (dst = 0; dst < PROC_MAX; dst++) {
> - /* Allocate maximum fd integers */
> - if ((pp->pp_pipes[dst] =
> - calloc(ps->ps_ninstances,
> - sizeof(int))) == NULL)
> - fatal("proc_init: calloc");
> + return (PROC_MAX);
> +}
>  
> - /* Mark fd as unused */
> - for (j = 0; j < ps->ps_ninstances; j++)
> - pp->pp_pipes[dst][j] = -1;
> - }
> - }
> - }
> +void
> +proc_exec(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
> +int argc, char **argv)
> +{
> + unsigned int proc, nargc, i, proc_i;

Re: snmpd(8): teach how to fork+exec

2016-10-21 Thread Rafael Zalamena
On Fri, Oct 14, 2016 at 06:47:09PM +0200, Rafael Zalamena wrote:
> On Mon, Sep 26, 2016 at 03:45:59PM +0200, Rafael Zalamena wrote:
> > Lets teach snmpd(8) how to fork+exec using the proc.c file from the latest
> > switchd(8) diff.
> > 
> > Note 1: I just tested the basic operations: startup and teardown.
> > Note 2: the kill with close will be implemented in another diff with the
> > ps_pid removal.
> > 
> 
> I've update the diff with the latest proc.c changes.
> 
> Note: I still have to implement kill with close().
> 

I got feedback from jca@ that the trap handler wasn't working, so after
trying to reproduce the problem myself I found one 'env' global variable
that was not being set and the child process was dying silently.
(thanks jca@ !)

Instead of depending on snmpe.c:snmpe env initialization (p_init), I'm
now calling smi_setenv() to do that in the main() function so all children
get the same behaviour. Also note that we don't have an 'extern' env in
smi.c anymore.

ok?

Index: proc.c
===
RCS file: /home/obsdcvs/src/usr.sbin/snmpd/proc.c,v
retrieving revision 1.20
diff -u -p -r1.20 proc.c
--- proc.c  7 Dec 2015 16:05:56 -   1.20
+++ proc.c  14 Oct 2016 15:42:19 -
@@ -1,7 +1,7 @@
 /* $OpenBSD: proc.c,v 1.20 2015/12/07 16:05:56 reyk Exp $  */
 
 /*
- * Copyright (c) 2010 - 2014 Reyk Floeter 
+ * Copyright (c) 2010 - 2016 Reyk Floeter 
  * Copyright (c) 2008 Pierre-Yves Ritschard 
  *
  * Permission to use, copy, modify, and distribute this software for any
@@ -22,6 +22,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -34,8 +35,12 @@
 
 #include "snmpd.h"
 
-voidproc_open(struct privsep *, struct privsep_proc *,
-   struct privsep_proc *, size_t);
+voidproc_exec(struct privsep *, struct privsep_proc *, unsigned int,
+   int, char **);
+voidproc_setup(struct privsep *, struct privsep_proc *, unsigned int);
+voidproc_open(struct privsep *, int, int);
+voidproc_accept(struct privsep *, int, enum privsep_procid,
+   unsigned int);
 voidproc_close(struct privsep *);
 int proc_ispeer(struct privsep_proc *, unsigned int, enum privsep_procid);
 voidproc_shutdown(struct privsep_proc *);
@@ -55,204 +60,383 @@ proc_ispeer(struct privsep_proc *procs, 
return (0);
 }
 
-void
-proc_init(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc)
+enum privsep_procid
+proc_getid(struct privsep_proc *procs, unsigned int nproc,
+const char *proc_name)
 {
-   unsigned int i, j, src, dst;
-   struct privsep_pipes*pp;
+   struct privsep_proc *p;
+   unsigned int proc;
 
-   /*
-* Allocate pipes for all process instances (incl. parent)
-*
-* - ps->ps_pipes: N:M mapping
-* N source processes connected to M destination processes:
-* [src][instances][dst][instances], for example
-* [PROC_RELAY][3][PROC_CA][3]
-*
-* - ps->ps_pp: per-process 1:M part of ps->ps_pipes
-* Each process instance has a destination array of socketpair fds:
-* [dst][instances], for example
-* [PROC_PARENT][0]
-*/
-   for (src = 0; src < PROC_MAX; src++) {
-   /* Allocate destination array for each process */
-   if ((ps->ps_pipes[src] = calloc(ps->ps_ninstances,
-   sizeof(struct privsep_pipes))) == NULL)
-   fatal("proc_init: calloc");
+   for (proc = 0; proc < nproc; proc++) {
+   p = &procs[proc];
+   if (strcmp(p->p_title, proc_name))
+   continue;
 
-   for (i = 0; i < ps->ps_ninstances; i++) {
-   pp = &ps->ps_pipes[src][i];
+   return (p->p_id);
+   }
 
-   for (dst = 0; dst < PROC_MAX; dst++) {
-   /* Allocate maximum fd integers */
-   if ((pp->pp_pipes[dst] =
-   calloc(ps->ps_ninstances,
-   sizeof(int))) == NULL)
-   fatal("proc_init: calloc");
+   return (PROC_MAX);
+}
 
-   /* Mark fd as unused */
-   for (j = 0; j < ps->ps_ninstances; j++)
-   pp->pp_pipes[dst][j] = -1;
-   }
-   }
-   }
+void
+proc_exec(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
+int argc, char **argv)
+{
+   unsigned int proc, nargc, i, proc_i;
+   char**nargv;
+   struct privsep_proc *p;
+   char num[32];
+   int  fd;
+
+   /* Prepare the new process argv. */
+   nargv = calloc(argc + 5, sizeof(char *));
+   if (nargv == NULL)
+  

Re: snmpd(8): teach how to fork+exec

2016-10-14 Thread Rafael Zalamena
On Mon, Sep 26, 2016 at 03:45:59PM +0200, Rafael Zalamena wrote:
> Lets teach snmpd(8) how to fork+exec using the proc.c file from the latest
> switchd(8) diff.
> 
> Note 1: I just tested the basic operations: startup and teardown.
> Note 2: the kill with close will be implemented in another diff with the
> ps_pid removal.
> 

I've update the diff with the latest proc.c changes.

Note: I still have to implement kill with close().

ok?

Index: proc.c
===
RCS file: /home/obsdcvs/src/usr.sbin/snmpd/proc.c,v
retrieving revision 1.20
diff -u -p -r1.20 proc.c
--- proc.c  7 Dec 2015 16:05:56 -   1.20
+++ proc.c  14 Oct 2016 15:42:19 -
@@ -1,7 +1,7 @@
 /* $OpenBSD: proc.c,v 1.20 2015/12/07 16:05:56 reyk Exp $  */
 
 /*
- * Copyright (c) 2010 - 2014 Reyk Floeter 
+ * Copyright (c) 2010 - 2016 Reyk Floeter 
  * Copyright (c) 2008 Pierre-Yves Ritschard 
  *
  * Permission to use, copy, modify, and distribute this software for any
@@ -22,6 +22,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -34,8 +35,12 @@
 
 #include "snmpd.h"
 
-voidproc_open(struct privsep *, struct privsep_proc *,
-   struct privsep_proc *, size_t);
+voidproc_exec(struct privsep *, struct privsep_proc *, unsigned int,
+   int, char **);
+voidproc_setup(struct privsep *, struct privsep_proc *, unsigned int);
+voidproc_open(struct privsep *, int, int);
+voidproc_accept(struct privsep *, int, enum privsep_procid,
+   unsigned int);
 voidproc_close(struct privsep *);
 int proc_ispeer(struct privsep_proc *, unsigned int, enum privsep_procid);
 voidproc_shutdown(struct privsep_proc *);
@@ -55,204 +60,383 @@ proc_ispeer(struct privsep_proc *procs, 
return (0);
 }
 
-void
-proc_init(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc)
+enum privsep_procid
+proc_getid(struct privsep_proc *procs, unsigned int nproc,
+const char *proc_name)
 {
-   unsigned int i, j, src, dst;
-   struct privsep_pipes*pp;
+   struct privsep_proc *p;
+   unsigned int proc;
 
-   /*
-* Allocate pipes for all process instances (incl. parent)
-*
-* - ps->ps_pipes: N:M mapping
-* N source processes connected to M destination processes:
-* [src][instances][dst][instances], for example
-* [PROC_RELAY][3][PROC_CA][3]
-*
-* - ps->ps_pp: per-process 1:M part of ps->ps_pipes
-* Each process instance has a destination array of socketpair fds:
-* [dst][instances], for example
-* [PROC_PARENT][0]
-*/
-   for (src = 0; src < PROC_MAX; src++) {
-   /* Allocate destination array for each process */
-   if ((ps->ps_pipes[src] = calloc(ps->ps_ninstances,
-   sizeof(struct privsep_pipes))) == NULL)
-   fatal("proc_init: calloc");
+   for (proc = 0; proc < nproc; proc++) {
+   p = &procs[proc];
+   if (strcmp(p->p_title, proc_name))
+   continue;
 
-   for (i = 0; i < ps->ps_ninstances; i++) {
-   pp = &ps->ps_pipes[src][i];
+   return (p->p_id);
+   }
 
-   for (dst = 0; dst < PROC_MAX; dst++) {
-   /* Allocate maximum fd integers */
-   if ((pp->pp_pipes[dst] =
-   calloc(ps->ps_ninstances,
-   sizeof(int))) == NULL)
-   fatal("proc_init: calloc");
+   return (PROC_MAX);
+}
 
-   /* Mark fd as unused */
-   for (j = 0; j < ps->ps_ninstances; j++)
-   pp->pp_pipes[dst][j] = -1;
-   }
-   }
-   }
+void
+proc_exec(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
+int argc, char **argv)
+{
+   unsigned int proc, nargc, i, proc_i;
+   char**nargv;
+   struct privsep_proc *p;
+   char num[32];
+   int  fd;
+
+   /* Prepare the new process argv. */
+   nargv = calloc(argc + 5, sizeof(char *));
+   if (nargv == NULL)
+   fatal("%s: calloc", __func__);
+
+   /* Copy call argument first. */
+   nargc = 0;
+   nargv[nargc++] = argv[0];
+
+   /* Set process name argument and save the position. */
+   nargv[nargc++] = "-P";
+   proc_i = nargc;
+   nargc++;
+
+   /* Point process instance arg to stack and copy the original args. */
+   nargv[nargc++] = "-I";
+   nargv[nargc++] = num;
+   for (i = 1; i < (unsigned int) argc; i++)
+   nargv[nargc++] = argv[i];
 
-   /*
-* Setup and run the parent