Re: [PATCH 1/2] VMD: Require interface to be defined in switches
On Mon, Oct 30, 2017 at 11:16:47PM +0100, Reyk Floeter wrote: > > > On 27.10.2017, at 19:10, Mike Larkinwrote: > > > > On Fri, Oct 27, 2017 at 09:58:09AM +0200, Martin Pieuchot wrote: > >> On 26/10/17(Thu) 23:47, Mike Larkin wrote: > >>> 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. > >> > >> I don't want to stop progress but I don't understand how creating a > >> bridge(4) beforehand help to "deconfigure" it. Hence my question. > > > > We want vmd(8) to be out of the bridge manipulation business, except for > > adding > > and removing tapX interfaces for the guest VMs it manages. > > > > Right now, vmd(8) is in the bridge creation and deletion business, and it is > > that second part that is giving us headache. Thus, we decided to remove both > > parts since we don't have a good solution as a whole. > > > > OK, that answers a question that I just asked in another mail. > > I still don't understand why this is improving anything, but since I'm not > around I cannot object. > > I used automatic bridge creation all the time and I never saw the theoretical > problem of not "deconfiguring bridges". Why does it have to? It is a > best-effort, it uses bridges if they already exist. And vmd already supported > configuration with pre-configured bridges. Or was there a (supposedly simple) > bug with pre-configured bridges? > > Reyk > This was a fix for what I outlined there as well as bizarre behaviour during vmctl reload with bridges defined.
Re: [PATCH 1/2] VMD: Require interface to be defined in switches
> On 27.10.2017, at 19:10, Mike Larkinwrote: > > On Fri, Oct 27, 2017 at 09:58:09AM +0200, Martin Pieuchot wrote: >> On 26/10/17(Thu) 23:47, Mike Larkin wrote: >>> 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. >> >> I don't want to stop progress but I don't understand how creating a >> bridge(4) beforehand help to "deconfigure" it. Hence my question. > > We want vmd(8) to be out of the bridge manipulation business, except for > adding > and removing tapX interfaces for the guest VMs it manages. > > Right now, vmd(8) is in the bridge creation and deletion business, and it is > that second part that is giving us headache. Thus, we decided to remove both > parts since we don't have a good solution as a whole. > OK, that answers a question that I just asked in another mail. I still don't understand why this is improving anything, but since I'm not around I cannot object. I used automatic bridge creation all the time and I never saw the theoretical problem of not "deconfiguring bridges". Why does it have to? It is a best-effort, it uses bridges if they already exist. And vmd already supported configuration with pre-configured bridges. Or was there a (supposedly simple) bug with pre-configured bridges? Reyk
Re: [PATCH 1/2] VMD: Require interface to be defined in switches
On Fri, Oct 27, 2017 at 09:58:09AM +0200, Martin Pieuchot wrote: > On 26/10/17(Thu) 23:47, Mike Larkin wrote: > > 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. > > I don't want to stop progress but I don't understand how creating a > bridge(4) beforehand help to "deconfigure" it. Hence my question. We want vmd(8) to be out of the bridge manipulation business, except for adding and removing tapX interfaces for the guest VMs it manages. Right now, vmd(8) is in the bridge creation and deletion business, and it is that second part that is giving us headache. Thus, we decided to remove both parts since we don't have a good solution as a whole. -ml
Re: [PATCH 1/2] VMD: Require interface to be defined in switches
On 26/10/17(Thu) 23:47, Mike Larkin wrote: > 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. I don't want to stop progress but I don't understand how creating a bridge(4) beforehand help to "deconfigure" it. Hence my question.
Re: [PATCH 1/2] VMD: Require interface to be defined in switches
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(>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, _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, ) < 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, ) < 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) > >
Re: [PATCH 1/2] VMD: Require interface to be defined in switches
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? > 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 intvcp_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(>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, _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, ) < 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, ) < 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, ) < 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, > - , sizeof(vfbr)); > -
[PATCH 1/2] VMD: Require interface to be defined in switches
* 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 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(>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, _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, ) < 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, ) < 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, ) < 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, - , sizeof(vfbr)); - proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFRDOMAIN, - , sizeof(vfbr)); proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFADD,