Re: [PATCH 1/2] VMD: complete switch configuration overhaul
On Thu, Nov 02, 2017 at 10:53:52AM -0700, Carlos Cardenas wrote:
> Removed configuration items on switches for:
> * adding interfaces
> * setting group
> * setting rdomain
>
> All of these items are now to be set in hostname.if and are
> queried by vmd for propagation to the vm's vif(s).
>
> Update example vm.conf and vm.conf manpage to reflect changes.
>
> Comments? Ok?
>
I'm not OK with this change.
I see that you want to remove the bridge configuration from vmd,
creating the bridge interface and adding static interfaces like em0,
but I do want to keep the ability to configure the dynamic interfaces
via vm.conf.
There is no reason to remove "group" and "rdomain" from the switch
context and it also exists in the vm/interface context. Inheriting it
from the hostname.if configuration is just a different thing - but I
still want be able to configure VMs in vm.conf.
switch "foo" {
group "bar" # put VM tapX interfaces in this group
rdomain 1 # put VM tapX interfaces in this rdomain
}
So I'm suggesting the following instead:
- You removed the functionality to create new bridge interfaces (DONE).
- You removed the "add" option to add static interfaces (DONE).
- Remove the code that changes group/rdomain of bridge or static interfaces.
- Keep the switch group/rdomain options to use it for VM interfaces.
So the diff would also be much smaller and you don't have to add these
regress tests that check if a switch is configured with group/rdomain.
There is no reason for ripping just everything out.
Reyk
> diff --git etc/examples/vm.conf etc/examples/vm.conf
> index f35235e3fba..836a9f5a7c7 100644
> --- etc/examples/vm.conf
> +++ etc/examples/vm.conf
> @@ -10,15 +10,14 @@ sets="/var/www/htdocs/pub/OpenBSD/snapshots/amd64/"
> #
>
> switch "uplink" {
> - # This interface will default to bridge0, but switch(4) is supported
> - #interface switch0
> -
> - # Add additional members
> - add em0
> + # This switch will use bridge0, defined by /etc/hostname.bridge0, as
> + # the underlying interface. switch(4) is also supported as
> + # interface switchX
> + interface bridge0
> }
>
> switch "local" {
> - add vether0
> + interface bridge1
> down
> }
>
> diff --git usr.sbin/vmd/parse.y usr.sbin/vmd/parse.y
> index a0e96545923..0bb1b0ceefa 100644
> --- usr.sbin/vmd/parse.y
> +++ usr.sbin/vmd/parse.y
> @@ -88,7 +88,6 @@ int parse_disk(char *);
> static struct vmop_create_params vmc;
> static struct vm_create_params *vcp;
> static struct vmd_switch *vsw;
> -static struct vmd_if *vif;
> static struct vmd_vm *vm;
> static char vsw_type[IF_NAMESIZE];
> static intvcp_disable;
> @@ -193,7 +192,7 @@ switch: SWITCH string {
> vsw->sw_id = env->vmd_nswitches + 1;
> vsw->sw_name = $2;
> vsw->sw_flags = VMIFF_UP;
> - TAILQ_INIT(>sw_ifs);
> + TAILQ_INIT(>sw_group);
>
> vcp_disable = 0;
> } '{' optnl switch_opts_l '}' {
> @@ -224,29 +223,6 @@ switch_opts_l: switch_opts_l switch_opts nl
> switch_opts : disable {
> vcp_disable = $1;
> }
> - | ADD string{
> - chartype[IF_NAMESIZE];
> -
> - if ((vif = calloc(1, sizeof(*vif))) == NULL)
> - fatal("could not allocate interface");
> -
> - if (priv_getiftype($2, type, NULL) == -1) {
> - yyerror("invalid interface: %s", $2);
> - free($2);
> - YYERROR;
> - }
> - vif->vif_name = $2;
> -
> - TAILQ_INSERT_TAIL(>sw_ifs, vif, vif_entry);
> - }
> - | GROUP string {
> - if (priv_validgroup($2) == -1) {
> - yyerror("invalid group name: %s", $2);
> - free($2);
> - YYERROR;
> - }
> - vsw->sw_group = $2;
> - }
> | INTERFACE string {
> if (priv_getiftype($2, vsw_type, NULL) == -1 ||
> priv_findname(vsw_type, vmd_descsw) == -1) {
> @@ -266,14 +242,6 @@ switch_opts : disable {
> | LOCKED LLADDR {
> vsw->sw_flags |= VMIFF_LOCKED;
> }
> - | RDOMAIN NUMBER{
> - if ($2 < 0 || $2 > RT_TABLEID_MAX) {
> - yyerror("invalid rdomain: %lld", $2);
> - YYERROR;
> - }
> -
[PATCH 1/2] VMD: complete switch configuration overhaul
Removed configuration items on switches for:
* adding interfaces
* setting group
* setting rdomain
All of these items are now to be set in hostname.if and are
queried by vmd for propagation to the vm's vif(s).
Update example vm.conf and vm.conf manpage to reflect changes.
Comments? Ok?
diff --git etc/examples/vm.conf etc/examples/vm.conf
index f35235e3fba..836a9f5a7c7 100644
--- etc/examples/vm.conf
+++ etc/examples/vm.conf
@@ -10,15 +10,14 @@ sets="/var/www/htdocs/pub/OpenBSD/snapshots/amd64/"
#
switch "uplink" {
- # This interface will default to bridge0, but switch(4) is supported
- #interface switch0
-
- # Add additional members
- add em0
+ # This switch will use bridge0, defined by /etc/hostname.bridge0, as
+ # the underlying interface. switch(4) is also supported as
+ # interface switchX
+ interface bridge0
}
switch "local" {
- add vether0
+ interface bridge1
down
}
diff --git usr.sbin/vmd/parse.y usr.sbin/vmd/parse.y
index a0e96545923..0bb1b0ceefa 100644
--- usr.sbin/vmd/parse.y
+++ usr.sbin/vmd/parse.y
@@ -88,7 +88,6 @@ intparse_disk(char *);
static struct vmop_create_params vmc;
static struct vm_create_params *vcp;
static struct vmd_switch *vsw;
-static struct vmd_if *vif;
static struct vmd_vm *vm;
static char vsw_type[IF_NAMESIZE];
static int vcp_disable;
@@ -193,7 +192,7 @@ switch : SWITCH string {
vsw->sw_id = env->vmd_nswitches + 1;
vsw->sw_name = $2;
vsw->sw_flags = VMIFF_UP;
- TAILQ_INIT(>sw_ifs);
+ TAILQ_INIT(>sw_group);
vcp_disable = 0;
} '{' optnl switch_opts_l '}' {
@@ -224,29 +223,6 @@ switch_opts_l : switch_opts_l switch_opts nl
switch_opts: disable {
vcp_disable = $1;
}
- | ADD string{
- chartype[IF_NAMESIZE];
-
- if ((vif = calloc(1, sizeof(*vif))) == NULL)
- fatal("could not allocate interface");
-
- if (priv_getiftype($2, type, NULL) == -1) {
- yyerror("invalid interface: %s", $2);
- free($2);
- YYERROR;
- }
- vif->vif_name = $2;
-
- TAILQ_INSERT_TAIL(>sw_ifs, vif, vif_entry);
- }
- | GROUP string {
- if (priv_validgroup($2) == -1) {
- yyerror("invalid group name: %s", $2);
- free($2);
- YYERROR;
- }
- vsw->sw_group = $2;
- }
| INTERFACE string {
if (priv_getiftype($2, vsw_type, NULL) == -1 ||
priv_findname(vsw_type, vmd_descsw) == -1) {
@@ -266,14 +242,6 @@ switch_opts: disable {
| LOCKED LLADDR {
vsw->sw_flags |= VMIFF_LOCKED;
}
- | RDOMAIN NUMBER{
- if ($2 < 0 || $2 > RT_TABLEID_MAX) {
- yyerror("invalid rdomain: %lld", $2);
- YYERROR;
- }
- vsw->sw_flags |= VMIFF_RDOMAIN;
- vsw->sw_rdomain = $2;
- }
| updown{
if ($1)
vsw->sw_flags |= VMIFF_UP;
diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
index d585bf75a99..6253ccdd785 100644
--- usr.sbin/vmd/priv.c
+++ usr.sbin/vmd/priv.c
@@ -44,6 +44,9 @@
#include "proc.h"
#include "vmd.h"
+/* if groups to ignore */
+const char *ifgroup_ignore[] = { "all", "bridge", "switch", NULL };
+
int priv_dispatch_parent(int, struct privsep_proc *, struct imsg *);
voidpriv_run(struct privsep *, struct privsep_proc *, void *);
@@ -264,6 +267,7 @@ vm_priv_ifconfig(struct privsep *ps, struct vmd_vm *vm)
struct vmd *env = ps->ps_env;
struct vm_create_params *vcp = >vm_params.vmc_params;
struct vmd_if *vif;
+ struct vmd_ifgr *vifgrp;
struct vmd_switch *vsw;
unsigned int i;
struct vmop_ifreqvfr, vfbr;
@@ -279,18 +283,6 @@ vm_priv_ifconfig(struct privsep *ps, struct vmd_vm *vm)
sizeof(vfr.vfr_name)) >= sizeof(vfr.vfr_name))
return (-1);
- /* Use the configured rdomain or get it from
