On Tue, Oct 10, 2017 at 10:52:42PM -0700, Mike Larkin wrote:
> On Wed, Oct 11, 2017 at 06:46:22AM +0200, Claudio Jeker wrote:
> > On Tue, Oct 10, 2017 at 03:57:25PM -0700, Carlos Cardenas wrote:
> > > Destroy switch on `vmctl reload` to allow SIOCBRDGADD to succeed
> > > when creating new bridge and attaching interfaces to it.
> > >
> > > Comments? Ok?
> >
> > I don't think it is a good idea to destroy and recreate bridge interfaces
> > on every reload. This changes the underlying network and may result in
> > broken connections and network hickups. I would suggest that vmctl is
> > instead checking which interfaces are on the bridge and calls SIOCBRDDEL
> > if needed and skips the SIOCBRDGADD if not needed.
> >
>
> you mean vmd, right?
>
> vmctl has no special privilege to do this.
yeah, sorry, was not fully awake yet. vmd needs to be smarter on config
reload IMO. Destroying interfaces for no reason is a bad practice.
> >
> > > diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
> > > index ef42549d105..0190c049837 100644
> > > --- usr.sbin/vmd/priv.c
> > > +++ usr.sbin/vmd/priv.c
> > > @@ -88,6 +88,7 @@ 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_IFDESTROY:
> > > case IMSG_VMDOP_PRIV_IFRDOMAIN:
> > > case IMSG_VMDOP_PRIV_IFADD:
> > > case IMSG_VMDOP_PRIV_IFUP:
> > > @@ -125,6 +126,13 @@ priv_dispatch_parent(int fd, struct privsep_proc *p,
> > > struct imsg *imsg)
> > > errno != EEXIST)
> > > log_warn("SIOCIFCREATE");
> > > break;
> > > + case IMSG_VMDOP_PRIV_IFDESTROY:
> > > + /* Destroy the bridge */
> > > + strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > > + if (ioctl(env->vmd_fd, SIOCIFDESTROY, &ifr) < 0 &&
> > > + errno != EEXIST)
> > > + log_warn("SIOCIFDESTROY");
> > > + break;
> > > case IMSG_VMDOP_PRIV_IFRDOMAIN:
> > > strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > > ifr.ifr_rdomainid = vfr.vfr_id;
> > > @@ -444,6 +452,23 @@ vm_priv_brconfig(struct privsep *ps, struct
> > > vmd_switch *vsw)
> > > return (0);
> > > }
> > >
> > > +int
> > > +vm_priv_brdestroy(struct privsep *ps, struct vmd_switch *vsw)
> > > +{
> > > + struct vmop_ifreq vfr;
> > > +
> > > + memset(&vfr, 0, sizeof(vfr));
> > > +
> > > + if (strlcpy(vfr.vfr_name, vsw->sw_ifname,
> > > + sizeof(vfr.vfr_name)) >= sizeof(vfr.vfr_name))
> > > + return (-1);
> > > +
> > > + proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFDESTROY,
> > > + &vfr, sizeof(vfr));
> > > +
> > > + return (0);
> > > +}
> > > +
> > > uint32_t
> > > vm_priv_addr(struct address *h, uint32_t vmid, int idx, int isvm)
> > > {
> > > diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> > > index f1abc54d9a3..834654a76de 100644
> > > --- usr.sbin/vmd/vmd.c
> > > +++ usr.sbin/vmd/vmd.c
> > > @@ -852,7 +852,7 @@ int
> > > vmd_reload(unsigned int reset, const char *filename)
> > > {
> > > struct vmd_vm *vm, *next_vm;
> > > - struct vmd_switch *vsw;
> > > + struct vmd_switch *vsw, *next_vsw;
> > > int reload = 0;
> > >
> > > /* Switch back to the default config file */
> > > @@ -885,6 +885,19 @@ vmd_reload(unsigned int reset, const char *filename)
> > > }
> > > }
> > >
> > > + TAILQ_FOREACH_SAFE(vsw, env->vmd_switches,
> > > + sw_entry, next_vsw) {
> > > + log_debug("%s: removing %s",
> > > + __func__, vsw->sw_ifname);
> > > + if (vm_priv_brdestroy(&env->vmd_ps,
> > > + vsw) == -1 ) {
> > > + log_warn("%s: failed to destroy switch",
> > > + __func__);
> > > + }
> > > + TAILQ_REMOVE(env->vmd_switches, vsw, sw_entry);
> > > + free(vsw);
> > > + }
> > > +
> > > /* Update shared global configuration in all children */
> > > if (config_setconfig(env) == -1)
> > > return (-1);
> > > diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> > > index 4b7b5f70495..fce5fcaaa60 100644
> > > --- usr.sbin/vmd/vmd.h
> > > +++ usr.sbin/vmd/vmd.h
> > > @@ -100,6 +100,7 @@ enum imsg_type {
> > > IMSG_VMDOP_PRIV_IFGROUP,
> > > IMSG_VMDOP_PRIV_IFADDR,
> > > IMSG_VMDOP_PRIV_IFRDOMAIN,
> > > + IMSG_VMDOP_PRIV_IFDESTROY,
> > > IMSG_VMDOP_VM_SHUTDOWN,
> > > IMSG_VMDOP_VM_REBOOT,
> > > IMSG_VMDOP_CONFIG
> > > @@ -323,6 +324,7 @@ int priv_findname(const char *, const char **);
> > > int priv_validgroup(const char *);
> > > int vm_priv_ifconfig(struct privsep *, struct vmd_vm *);
> > > int vm_priv_brconfig(struct privsep *, struct vmd_switch *);
> > > +int vm_priv_brdestroy(struct privsep *, struct vmd_switch *);
> > > uint32_t vm_priv_addr(struct address *, uint32_t, int, int);
> > >
> > > /* vmm.c */
> > > --
> > > 2.14.2
> > >
> >
> > --
> > :wq Claudio
> >
>
--
:wq Claudio
