vmd(8) sends global config too late

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

vmd(8) sends global config too late

Martijn van Duren-5
Hello tech@,

When playing around with local prefix in vm.conf(5) I noticed that it
wasn't picked up by my vm. The reason for this was that config_setvm was
called before config_setconfig. Diff below fixes this.

OK?

martijn@

Index: vmd.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
retrieving revision 1.80
diff -u -p -r1.80 vmd.c
--- vmd.c 18 Feb 2018 01:00:25 -0000 1.80
+++ vmd.c 21 Feb 2018 22:26:35 -0000
@@ -821,6 +821,10 @@ vmd_configure(void)
  exit(1);
  }
 
+ /* Send shared global configuration to all children */
+ if (config_setconfig(env) == -1)
+ return (-1);
+
  if (env->vmd_noaction) {
  fprintf(stderr, "configuration OK\n");
  proc_kill(&env->vmd_ps);
@@ -848,10 +852,6 @@ vmd_configure(void)
  if (config_setvm(&env->vmd_ps, vm, -1, vm->vm_params.vmc_uid) == -1)
  return (-1);
  }
-
- /* Send shared global configuration to all children */
- if (config_setconfig(env) == -1)
- return (-1);
 
  return (0);
 }

Reply | Threaded
Open this post in threaded view
|

Re: vmd(8) sends global config too late

Carlos Cardenas
On Wed, Feb 21, 2018 at 11:27:05PM +0100, Martijn van Duren wrote:
> Hello tech@,
>
> When playing around with local prefix in vm.conf(5) I noticed that it
> wasn't picked up by my vm. The reason for this was that config_setvm was
> called before config_setconfig. Diff below fixes this.
>
> OK?
>
> martijn@

Howdy.

I'm confused by the problem statement and use case(s) you are referring
to so let me rephrase what I think you might be saying:
* when a custom prefix is defined in vm.conf, local interfaces doesn't
  work on vms that are not disabled by default

Is that correct?

When I put a custom prefix in vm.conf and:
* launch vms via vmctl
or
* start vms that do not start up automatically
the custom prefix is used.

My comments on this patch are:
* the calling of config_setconfig is called too early; it needs to
  happen after the "env->vmd_noaction" check
* if we had an issue with prefix being sent too late on vmd startup,
  then we'll also have an issue in vmd_reload as well. The
  config_setconfig call needs to happen after we parse the new config
  file.

For testing, can you ensure the following cases work as designed:
1) launching vms via vmctl
2) start vms that do not start automatically
3) vms that do start automatically
4) Modify prefix in vm.conf, `vmctl reload`, and ensure the above three items work
   with the new prefix.

Thanks.

+--+
Carlos

>
> Index: vmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 vmd.c
> --- vmd.c 18 Feb 2018 01:00:25 -0000 1.80
> +++ vmd.c 21 Feb 2018 22:26:35 -0000
> @@ -821,6 +821,10 @@ vmd_configure(void)
>   exit(1);
>   }
>  
> + /* Send shared global configuration to all children */
> + if (config_setconfig(env) == -1)
> + return (-1);
> +
>   if (env->vmd_noaction) {
>   fprintf(stderr, "configuration OK\n");
>   proc_kill(&env->vmd_ps);
> @@ -848,10 +852,6 @@ vmd_configure(void)
>   if (config_setvm(&env->vmd_ps, vm, -1, vm->vm_params.vmc_uid) == -1)
>   return (-1);
>   }
> -
> - /* Send shared global configuration to all children */
> - if (config_setconfig(env) == -1)
> - return (-1);
>  
>   return (0);
>  }
>

Reply | Threaded
Open this post in threaded view
|

Re: vmd(8) sends global config too late

Martijn van Duren-5
Hello Carlos,

On 02/22/18 03:30, Carlos Cardenas wrote:

> On Wed, Feb 21, 2018 at 11:27:05PM +0100, Martijn van Duren wrote:
>> Hello tech@,
>>
>> When playing around with local prefix in vm.conf(5) I noticed that it
>> wasn't picked up by my vm. The reason for this was that config_setvm was
>> called before config_setconfig. Diff below fixes this.
>>
>> OK?
>>
>> martijn@
>
> Howdy.
>
> I'm confused by the problem statement and use case(s) you are referring
> to so let me rephrase what I think you might be saying:
> * when a custom prefix is defined in vm.conf, local interfaces doesn't
>   work on vms that are not disabled by default
>
> Is that correct?

Yes
>
> When I put a custom prefix in vm.conf and:
> * launch vms via vmctl
> or
> * start vms that do not start up automatically
> the custom prefix is used.

Also correct
>
> My comments on this patch are:
> * the calling of config_setconfig is called too early; it needs to
>   happen after the "env->vmd_noaction" check

Fixed
> * if we had an issue with prefix being sent too late on vmd startup,
>   then we'll also have an issue in vmd_reload as well. The
>   config_setconfig call needs to happen after we parse the new config
>   file.

Fixed
>
> For testing, can you ensure the following cases work as designed:
> 1) launching vms via vmctl
> 2) start vms that do not start automatically
> 3) vms that do start automatically
> 4) Modify prefix in vm.conf, `vmctl reload`, and ensure the above three items work
>    with the new prefix.

Done
>
> Thanks.
>
> +--+
> Carlos

Index: vmd.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
retrieving revision 1.79
diff -u -p -r1.79 vmd.c
--- vmd.c 10 Jan 2018 14:59:59 -0000 1.79
+++ vmd.c 9 Mar 2018 18:06:15 -0000
@@ -823,6 +823,10 @@ vmd_configure(void)
  exit(0);
  }
 
+ /* Send shared global configuration to all children */
+ if (config_setconfig(env) == -1)
+ return (-1);
+
  TAILQ_FOREACH(vsw, env->vmd_switches, sw_entry) {
  if (vsw->sw_running)
  continue;
@@ -845,10 +849,6 @@ vmd_configure(void)
  return (-1);
  }
 
- /* Send shared global configuration to all children */
- if (config_setconfig(env) == -1)
- return (-1);
-
  return (0);
 }
 
@@ -888,16 +888,18 @@ vmd_reload(unsigned int reset, const cha
  vm_remove(vm);
  }
  }
-
- /* Update shared global configuration in all children */
- if (config_setconfig(env) == -1)
- return (-1);
  }
 
  if (parse_config(filename) == -1) {
  log_debug("%s: failed to load config file %s",
     __func__, filename);
  return (-1);
+ }
+
+ if (reload) {
+ /* Update shared global configuration in all children */
+ if (config_setconfig(env) == -1)
+ return (-1);
  }
 
  TAILQ_FOREACH(vsw, env->vmd_switches, sw_entry) {

Reply | Threaded
Open this post in threaded view
|

Re: vmd(8) sends global config too late

Carlos Cardenas
On Fri, Mar 09, 2018 at 07:06:44PM +0100, Martijn van Duren wrote:

> Hello Carlos,
>
> On 02/22/18 03:30, Carlos Cardenas wrote:
> > On Wed, Feb 21, 2018 at 11:27:05PM +0100, Martijn van Duren wrote:
> >> Hello tech@,
> >>
> >> When playing around with local prefix in vm.conf(5) I noticed that it
> >> wasn't picked up by my vm. The reason for this was that config_setvm was
> >> called before config_setconfig. Diff below fixes this.
> >>
> >> OK?
> >>
> >> martijn@
> >
> > Howdy.
> >
> > I'm confused by the problem statement and use case(s) you are referring
> > to so let me rephrase what I think you might be saying:
> > * when a custom prefix is defined in vm.conf, local interfaces doesn't
> >   work on vms that are not disabled by default
> >
> > Is that correct?
>
> Yes
> >
> > When I put a custom prefix in vm.conf and:
> > * launch vms via vmctl
> > or
> > * start vms that do not start up automatically
> > the custom prefix is used.
>
> Also correct
> >
> > My comments on this patch are:
> > * the calling of config_setconfig is called too early; it needs to
> >   happen after the "env->vmd_noaction" check
>
> Fixed
> > * if we had an issue with prefix being sent too late on vmd startup,
> >   then we'll also have an issue in vmd_reload as well. The
> >   config_setconfig call needs to happen after we parse the new config
> >   file.
>
> Fixed
> >
> > For testing, can you ensure the following cases work as designed:
> > 1) launching vms via vmctl
> > 2) start vms that do not start automatically
> > 3) vms that do start automatically
> > 4) Modify prefix in vm.conf, `vmctl reload`, and ensure the above three items work
> >    with the new prefix.
>
> Done
> >
> > Thanks.
> >
> > +--+
> > Carlos

This patch looks good.

Thanks again Martijn.

OK ccardenas@

+--+
Carlos

>
> Index: vmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 vmd.c
> --- vmd.c 10 Jan 2018 14:59:59 -0000 1.79
> +++ vmd.c 9 Mar 2018 18:06:15 -0000
> @@ -823,6 +823,10 @@ vmd_configure(void)
>   exit(0);
>   }
>  
> + /* Send shared global configuration to all children */
> + if (config_setconfig(env) == -1)
> + return (-1);
> +
>   TAILQ_FOREACH(vsw, env->vmd_switches, sw_entry) {
>   if (vsw->sw_running)
>   continue;
> @@ -845,10 +849,6 @@ vmd_configure(void)
>   return (-1);
>   }
>  
> - /* Send shared global configuration to all children */
> - if (config_setconfig(env) == -1)
> - return (-1);
> -
>   return (0);
>  }
>  
> @@ -888,16 +888,18 @@ vmd_reload(unsigned int reset, const cha
>   vm_remove(vm);
>   }
>   }
> -
> - /* Update shared global configuration in all children */
> - if (config_setconfig(env) == -1)
> - return (-1);
>   }
>  
>   if (parse_config(filename) == -1) {
>   log_debug("%s: failed to load config file %s",
>      __func__, filename);
>   return (-1);
> + }
> +
> + if (reload) {
> + /* Update shared global configuration in all children */
> + if (config_setconfig(env) == -1)
> + return (-1);
>   }
>  
>   TAILQ_FOREACH(vsw, env->vmd_switches, sw_entry) {