On Sun, Nov 18, 2018 at 11:40:40PM +0100, Remi Locherer wrote:
> Hi,
>
> from the discussion I understand nobody rejects the functionality.
>
> To ease the review here again the diff (with incorporated feedback from
> anton@ (redundant parens)).
>
> Any comments or OKs?
Not sure about this, since control_setup() is called by reconfigure().
So it is possible that bgpd will terminate while reloading the config.
I would prefer if control_setup() would return an error that is passed
back up to reconfigure() instead of the fatalx() call. In that case bgpd
will quit on startup but return a failure on reload bit keep on running
with the old config. This would also allow to do the same check on the
restricted socket. I also added some minor comments below.
--
:wq Claudio
> Remi
>
>
> cvs diff: Diffing .
> Index: bgpd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
> retrieving revision 1.204
> diff -u -p -r1.204 bgpd.c
> --- bgpd.c 29 Sep 2018 08:11:11 -0000 1.204
> +++ bgpd.c 18 Nov 2018 22:21:51 -0000
> @@ -939,6 +939,8 @@ control_setup(struct bgpd_config *conf)
> }
> if ((cname = strdup(conf->csock)) == NULL)
> fatal("strdup");
> + if (control_check(cname) == -1)
> + fatalx("control socket check failed");
> if ((fd = control_init(0, cname)) == -1)
> fatalx("control socket setup failed");
> if (control_listen(fd) == -1)
> Index: control.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/control.c,v
> retrieving revision 1.90
> diff -u -p -r1.90 control.c
> --- control.c 11 Aug 2017 16:02:53 -0000 1.90
> +++ control.c 10 Nov 2018 20:52:46 -0000
> @@ -38,6 +38,32 @@ void control_result(struct ctl_conn *,
> ssize_t imsg_read_nofd(struct imsgbuf *);
>
> int
> +control_check(char *path)
> +{
> + struct sockaddr_un sun;
> + int fd;
> +
> + bzero(&sun, sizeof(sun));
> + sun.sun_family = AF_UNIX;
> + strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
> +
> + if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
> + log_warn("control_check: socket check");
You can use __func__ for the function name here. I also would remove
'check' from the string.
> + return (-1);
> + }
> +
> + if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) {
> + log_warnx("control_check: socket in use");
Same __func__ change here.
> + close(fd);
> + return (-1);
> + }
> +
> + close(fd);
> +
> + return (0);
> +}
> +
> +int
> control_init(int restricted, char *path)
> {
> struct sockaddr_un sun;
> Index: session.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.h,v
> retrieving revision 1.125
> diff -u -p -r1.125 session.h
> --- session.h 24 Oct 2018 08:26:37 -0000 1.125
> +++ session.h 10 Nov 2018 21:53:04 -0000
> @@ -250,6 +250,7 @@ void prepare_listeners(struct bgpd_conf
> int get_mpe_label(struct rdomain *);
>
> /* control.c */
> +int control_check(char *);
> int control_init(int, char *);
> int control_listen(int);
> void control_shutdown(int);
>
