On Fri, Oct 27, 2017 at 08:23:05AM +0200, Martin Pieuchot wrote:
> On 26/10/17(Thu) 16:23, Carlos Cardenas wrote:
> > * Require interface name to be defined for switches in vm.conf
> > ** Requires user to create bridge(4) beforehand
> > * Remove code to create bridges on the fly
> > * Add code to ensure bridge really exists
> > * Update manpage switch and example sections
>
> What's the motivation to ask people to create a bridge(4) beforehand?
>
This discussion came out of a previous effort to fix a bug where vmd(8) doesn't
deconfigure bridges (and/or undo changes to bridges) it made during
initialization. After discussing this a bit with claudio, we decided that
rather than having vmd try to do magic stuff with creating bridges and
tearing them down behind the scenes (including corner cases where vmd is
killed or crashes/etc), it made sense instead to say "you create the bridge
and tell me which one you want to use". This can easily be accomplished
with a 1 (or 2) line /etc/hostname.bridgeX file.
We may decide later to have vmd try to create bridges again at some point
but for now, I think this is a way to make some forward progress in this
area.
-ml
> > diff --git usr.sbin/vmd/parse.y usr.sbin/vmd/parse.y
> > index 55a9b0c7acc..a0b21fa8ef1 100644
> > --- usr.sbin/vmd/parse.y
> > +++ usr.sbin/vmd/parse.y
> > @@ -90,7 +90,6 @@ static struct vm_create_params *vcp;
> > static struct vmd_switch *vsw;
> > static struct vmd_if *vif;
> > static struct vmd_vm *vm;
> > -static unsigned int vsw_unit;
> > static char vsw_type[IF_NAMESIZE];
> > static int vcp_disable;
> > static size_t vcp_nnics;
> > @@ -194,12 +193,17 @@ switch : SWITCH string
> > {
> > vsw->sw_id = env->vmd_nswitches + 1;
> > vsw->sw_name = $2;
> > vsw->sw_flags = VMIFF_UP;
> > - snprintf(vsw->sw_ifname, sizeof(vsw->sw_ifname),
> > - "%s%u", vsw_type, vsw_unit++);
> > TAILQ_INIT(&vsw->sw_ifs);
> >
> > vcp_disable = 0;
> > } '{' optnl switch_opts_l '}' {
> > + if (strnlen(vsw->sw_ifname,
> > + sizeof(vsw->sw_ifname)) == 0) {
> > + yyerror("switch \"%s\" is missing interface
> > name",
> > + vsw->sw_name);
> > + YYERROR;
> > + }
> > +
> > if (vcp_disable) {
> > log_debug("%s:%d: switch \"%s\""
> > " skipped (disabled)",
> > @@ -244,13 +248,12 @@ switch_opts : disable {
> > vsw->sw_group = $2;
> > }
> > | INTERFACE string {
> > - if (priv_getiftype($2, vsw_type, &vsw_unit) == -1 ||
> > + if (priv_getiftype($2, vsw_type, NULL) == -1 ||
> > priv_findname(vsw_type, vmd_descsw) == -1) {
> > yyerror("invalid switch interface: %s", $2);
> > free($2);
> > YYERROR;
> > }
> > - vsw_unit++;
> >
> > if (strlcpy(vsw->sw_ifname, $2,
> > sizeof(vsw->sw_ifname)) >= sizeof(vsw->sw_ifname)) {
> > diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
> > index ef42549d105..d66bdcc9b4f 100644
> > --- usr.sbin/vmd/priv.c
> > +++ usr.sbin/vmd/priv.c
> > @@ -87,8 +87,8 @@ priv_dispatch_parent(int fd, struct privsep_proc *p,
> > struct imsg *imsg)
> >
> > switch (imsg->hdr.type) {
> > case IMSG_VMDOP_PRIV_IFDESCR:
> > - case IMSG_VMDOP_PRIV_IFCREATE:
> > case IMSG_VMDOP_PRIV_IFRDOMAIN:
> > + case IMSG_VMDOP_PRIV_IFEXISTS:
> > case IMSG_VMDOP_PRIV_IFADD:
> > case IMSG_VMDOP_PRIV_IFUP:
> > case IMSG_VMDOP_PRIV_IFDOWN:
> > @@ -118,13 +118,6 @@ priv_dispatch_parent(int fd, struct privsep_proc *p,
> > struct imsg *imsg)
> > if (ioctl(env->vmd_fd, SIOCSIFDESCR, &ifr) < 0)
> > log_warn("SIOCSIFDESCR");
> > break;
> > - case IMSG_VMDOP_PRIV_IFCREATE:
> > - /* Create the bridge if it doesn't exist */
> > - strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > - if (ioctl(env->vmd_fd, SIOCIFCREATE, &ifr) < 0 &&
> > - errno != EEXIST)
> > - log_warn("SIOCIFCREATE");
> > - break;
> > case IMSG_VMDOP_PRIV_IFRDOMAIN:
> > strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > ifr.ifr_rdomainid = vfr.vfr_id;
> > @@ -145,6 +138,13 @@ priv_dispatch_parent(int fd, struct privsep_proc *p,
> > struct imsg *imsg)
> > errno != EEXIST)
> > log_warn("SIOCBRDGADD");
> > break;
> > + case IMSG_VMDOP_PRIV_IFEXISTS:
> > + /* Determine if bridge/switch exists */
> > + strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > + if (ioctl(env->vmd_fd, SIOCGIFFLAGS, &ifr) < 0)
> > + fatalx("%s: bridge \"%s\" does not exist",
> > + __func__, vfr.vfr_name);
> > + break;
> > case IMSG_VMDOP_PRIV_IFUP:
> > case IMSG_VMDOP_PRIV_IFDOWN:
> > /* Set the interface status */
> > @@ -319,10 +319,6 @@ vm_priv_ifconfig(struct privsep *ps, struct vmd_vm *vm)
> > log_debug("%s: interface %s add %s", __func__,
> > vfbr.vfr_name, vfbr.vfr_value);
> >
> > - proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFCREATE,
> > - &vfbr, sizeof(vfbr));
> > - proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFRDOMAIN,
> > - &vfbr, sizeof(vfbr));
> > proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFADD,
> > &vfbr, sizeof(vfbr));
> > } else if (vif->vif_switch != NULL)
> > @@ -398,7 +394,8 @@ vm_priv_brconfig(struct privsep *ps, struct vmd_switch
> > *vsw)
> > sizeof(vfr.vfr_name)) >= sizeof(vfr.vfr_name))
> > return (-1);
> >
> > - proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFCREATE,
> > + // ensure bridge/switch exists
> > + proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFEXISTS,
> > &vfr, sizeof(vfr));
> >
> > /* Use the configured rdomain or get it from the process */
> > diff --git usr.sbin/vmd/vm.conf.5 usr.sbin/vmd/vm.conf.5
> > index 0927fbfecf9..f4bfc2f215b 100644
> > --- usr.sbin/vmd/vm.conf.5
> > +++ usr.sbin/vmd/vm.conf.5
> > @@ -279,11 +279,6 @@ Set the
> > or
> > .Xr bridge 4
> > network interface of this switch.
> > -If not specified,
> > -.Ar bridge0
> > -will be used where the interface unit will be incremented for each switch,
> > -e.g.\&
> > -.Ar bridge0 , bridge1 , ...
> > If the type is changed to
> > .Ar switch0 ,
> > it will be used for each following switch.
> > @@ -318,6 +313,7 @@ vm "vm2.example.com" {
> > Create the switch "uplink" with an additional physical network interface:
> > .Bd -literal -offset indent
> > switch "uplink" {
> > + interface bridge0
> > add em0
> > }
> > .Ed
> > diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> > index 4b7b5f70495..e02c2ea7882 100644
> > --- usr.sbin/vmd/vmd.h
> > +++ usr.sbin/vmd/vmd.h
> > @@ -94,7 +94,7 @@ enum imsg_type {
> > IMSG_VMDOP_RELOAD,
> > IMSG_VMDOP_PRIV_IFDESCR,
> > IMSG_VMDOP_PRIV_IFADD,
> > - IMSG_VMDOP_PRIV_IFCREATE,
> > + IMSG_VMDOP_PRIV_IFEXISTS,
> > IMSG_VMDOP_PRIV_IFUP,
> > IMSG_VMDOP_PRIV_IFDOWN,
> > IMSG_VMDOP_PRIV_IFGROUP,
> > --
> > 2.14.2
> >
>
