On 10/10/17 23:31, Claudio Jeker wrote:
> 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.
>
We can definitely make things smarter. For my sake, I'm going to go through a
couple current scenarios to make sure I'm picking up what you're putting down.
1) vm.conf has the following switch definition
switch "a" {
add vether0
}
On vmd startup, a bridge(4) is created (bridge0 at first) and vether0 is added
to it. On reset/reload, any running vms are stopped and the switch is removed
prior to re-reading the config file and creating new bridges/vms. This means,
the new bridge is now bridge1 and vether0 will be added.
Today, this scenario fails since vether0 was never removed from bridge0 to be
added to bridge1. (what the patch addresses).
2) vm.conf has the following definition
switch "a" {
interface "bridge0"
add vether0
}
Just like 1) except this time after any reset/reload, the switch name is
always bridge0. This seems to be way a switch(4) is used in vmd.
In scenario 1, are you proposing we keep bridge0 (and other predecessors around)
and only remove/add the bridge ports (vether0 in this case) to the current
bridge{n}? If so, should we use the bridge ports defined in vm.conf or from
SIOCBRDGIFS since they could be different?
How should we handle scenario 2 since my patch is kinda heavy handed for it?
Another option would be to make "interface NAME" mandatory in switch definitions
which collapses our scenarios.
What are y'all's thoughts?
+--+
Carlos
>>>
>>>> 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
>>>
>>
>
