Re: [PATCH 1/2 v2] VMD: remove add from switch configuration
On Thu, Nov 02, 2017 at 05:55:32PM -0700, Carlos Cardenas wrote:
> Remove configuration items on switches for:
> * adding static interfaces
>
> Adding static interfaces are to be set in hostname.if.
>
> Changed rule on rdomain:
> * vm->interface->rdomain takes precedence over sw->rdomain
>
> Update examples/vm.conf and vm.conf manpage to reflect changes.
>
> Comments? Ok?
>
Style comments below, otherwise OK
Thanks!
Reyk
> diff --git etc/examples/vm.conf etc/examples/vm.conf
> index f35235e3fba..d61ce8c4977 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
> + # This switch will use bridge0, defined by /etc/hostname.bridge0, as
> + # the underlying interface. switch(4) is also supported
> #interface switch0
> -
> - # Add additional members
> - add em0
> + interface bridge0
> }
>
> switch "local" {
> - add vether0
> + interface bridge1
> down
> }
>
> diff --git usr.sbin/vmd/parse.y usr.sbin/vmd/parse.y
> index a0e96545923..18543a74911 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,6 @@ switch: SWITCH string {
> vsw->sw_id = env->vmd_nswitches + 1;
> vsw->sw_name = $2;
> vsw->sw_flags = VMIFF_UP;
> - TAILQ_INIT(>sw_ifs);
>
> vcp_disable = 0;
> } '{' optnl switch_opts_l '}' {
> @@ -224,21 +222,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);
> diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
> index d585bf75a99..c099bdf4ba5 100644
> --- usr.sbin/vmd/priv.c
> +++ usr.sbin/vmd/priv.c
> @@ -255,7 +255,17 @@ priv_validgroup(const char *name)
> }
>
> /*
> - * Called from the process peer
> + * Called from the Parent process to setup vm interface(s)
> + * - ensure the interface has the description set (tracking purposes)
> + * - if interface is to be attached to a switch, attach it
> + * - check if rdomain is set on interface and switch
> + * - if interface only or both, use interface rdomain
> + * - if switch only, use switch rdomain
> + * - check if group is set on interface and switch
> + * - if interface, add it
> + * - if switch, add it
> + * - ensure the interface is up/down
> + * - if local interface, set address
> */
Just a style note -
The "Called from the process peer" was a comment for the "section" of
vm_priv_* functions - that is why it is followed by an empty newline.
If you want to add a "mlarkin-style" comment for the function, please
add it additionally without an empty newline between comment and the
function header. I don't always add such function comments, but when
we do, we should do it in a consistent way like mlarkin did.
Please also note that this comment might diverge from reality at some
point. It is a detailed description of what happens in the code, but
what if somebody changes the code without adjusting the comment? I'd
say it is better to describe the steps inline... but I don't insist.
>
> int
> @@ -279,18 +289,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 the process */
> - if (vif->vif_flags & VMIFF_RDOMAIN)
> - vfr.vfr_id = vif->vif_rdomain;
> - else
> - vfr.vfr_id = getrtable();
> - if (vfr.vfr_id != 0)
> -
[PATCH 1/2 v2] VMD: remove add from switch configuration
Remove configuration items on switches for:
* adding static interfaces
Adding static interfaces are to be set in hostname.if.
Changed rule on rdomain:
* vm->interface->rdomain takes precedence over sw->rdomain
Update examples/vm.conf and vm.conf manpage to reflect changes.
Comments? Ok?
diff --git etc/examples/vm.conf etc/examples/vm.conf
index f35235e3fba..d61ce8c4977 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
+ # This switch will use bridge0, defined by /etc/hostname.bridge0, as
+ # the underlying interface. switch(4) is also supported
#interface switch0
-
- # Add additional members
- add em0
+ interface bridge0
}
switch "local" {
- add vether0
+ interface bridge1
down
}
diff --git usr.sbin/vmd/parse.y usr.sbin/vmd/parse.y
index a0e96545923..18543a74911 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,6 @@ switch : SWITCH string {
vsw->sw_id = env->vmd_nswitches + 1;
vsw->sw_name = $2;
vsw->sw_flags = VMIFF_UP;
- TAILQ_INIT(>sw_ifs);
vcp_disable = 0;
} '{' optnl switch_opts_l '}' {
@@ -224,21 +222,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);
diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
index d585bf75a99..c099bdf4ba5 100644
--- usr.sbin/vmd/priv.c
+++ usr.sbin/vmd/priv.c
@@ -255,7 +255,17 @@ priv_validgroup(const char *name)
}
/*
- * Called from the process peer
+ * Called from the Parent process to setup vm interface(s)
+ * - ensure the interface has the description set (tracking purposes)
+ * - if interface is to be attached to a switch, attach it
+ * - check if rdomain is set on interface and switch
+ * - if interface only or both, use interface rdomain
+ * - if switch only, use switch rdomain
+ * - check if group is set on interface and switch
+ * - if interface, add it
+ * - if switch, add it
+ * - ensure the interface is up/down
+ * - if local interface, set address
*/
int
@@ -279,18 +289,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 the process */
- if (vif->vif_flags & VMIFF_RDOMAIN)
- vfr.vfr_id = vif->vif_rdomain;
- else
- vfr.vfr_id = getrtable();
- if (vfr.vfr_id != 0)
- log_debug("%s: interface %s rdomain %u", __func__,
- vfr.vfr_name, vfr.vfr_id);
-
- proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFRDOMAIN,
- , sizeof(vfr));
-
/* Description can be truncated */
(void)snprintf(vfr.vfr_value, sizeof(vfr.vfr_value),
"vm%u-if%u-%s", vm->vm_vmid, i, vcp->vcp_name);
@@ -301,8 +299,16 @@ vm_priv_ifconfig(struct privsep *ps, struct vmd_vm *vm)
proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFDESCR,
, sizeof(vfr));
- /* Add interface to bridge/switch */
- if ((vsw = switch_getbyname(vif->vif_switch)) != NULL) {
+ vsw = switch_getbyname(vif->vif_switch);
+ /* set default rdomain */
+ vfr.vfr_id = getrtable();
+
+ /* Check if switch should exist */
+ if (vsw == NULL &&
