ldomctl: Fail on duplicate vcpu and memory parameters

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

ldomctl: Fail on duplicate vcpu and memory parameters

Klemens Nanni-2
Domains get to define their cores and memory only once unlike the other
parameters of which it makes sense to have more than one.

        $ cat dup.conf
        domain primary {
                vcpu 2
                vcpu 2
        }
        $ ldomctl init-system -n dup.conf ; echo $?
        0
        $ ./obj/ldomctl init-system -n dup.conf
        dup.conf:3 duplicate vcpu option

OK?


Index: parse.y
===================================================================
RCS file: /cvs/src/usr.sbin/ldomctl/parse.y,v
retrieving revision 1.13
diff -u -p -r1.13 parse.y
--- parse.y 28 Nov 2019 18:40:42 -0000 1.13
+++ parse.y 5 Jan 2020 18:03:54 -0000
@@ -138,10 +155,18 @@ domainoptsl : domainopts nl
  ;
 
 domainopts : VCPU vcpu {
+ if (domain->vcpu) {
+ yyerror("duplicate vcpu option");
+ YYERROR;
+ }
  domain->vcpu = $2.count;
  domain->vcpu_stride = $2.stride;
  }
  | MEMORY memory {
+ if (domain->memory) {
+ yyerror("duplicate memory option");
+ YYERROR;
+ }
  domain->memory = $2;
  }
  | VDISK STRING {

Reply | Threaded
Open this post in threaded view
|

Re: ldomctl: Fail on duplicate vcpu and memory parameters

Klemens Nanni-2
On Sun, Jan 05, 2020 at 07:09:46PM +0100, Klemens Nanni wrote:

> Domains get to define their cores and memory only once unlike the other
> parameters of which it makes sense to have more than one.
>
> $ cat dup.conf
> domain primary {
> vcpu 2
> vcpu 2
> }
> $ ldomctl init-system -n dup.conf ; echo $?
> 0
> $ ./obj/ldomctl init-system -n dup.conf
> dup.conf:3 duplicate vcpu option
>
> OK?
Now that checks for mandatory options are in, this somewhat completes
it at the other end.

Here's the same diff with an additional check for duplicate iodevices,
only that those must be globally unique (just like domain names).

OK?


Index: parse.y
===================================================================
RCS file: /cvs/src/usr.sbin/ldomctl/parse.y,v
retrieving revision 1.15
diff -u -p -r1.15 parse.y
--- parse.y 9 Jan 2020 22:06:23 -0000 1.15
+++ parse.y 13 Jan 2020 11:44:45 -0000
@@ -156,10 +156,18 @@ domainoptsl : domainopts nl
  ;
 
 domainopts : VCPU vcpu {
+ if (domain->vcpu) {
+ yyerror("duplicate vcpu option");
+ YYERROR;
+ }
  domain->vcpu = $2.count;
  domain->vcpu_stride = $2.stride;
  }
  | MEMORY memory {
+ if (domain->memory) {
+ yyerror("duplicate memory option");
+ YYERROR;
+ }
  domain->memory = $2;
  }
  | VDISK STRING {
@@ -180,10 +188,19 @@ domainopts : VCPU vcpu {
  SIMPLEQ_INSERT_TAIL(&domain->var_list, var, entry);
  }
  | IODEVICE STRING {
- struct iodev *iodev = xmalloc(sizeof(struct iodev));
+ struct domain *odomain;
+ struct iodev *iodev;
+ SIMPLEQ_FOREACH(odomain, &conf->domain_list, entry)
+ SIMPLEQ_FOREACH(iodev, &odomain->iodev_list, entry)
+ if (strcmp(iodev->path, $2) == 0) {
+ yyerror("iodevice assigned"
+    " twice: %s", $2);
+ YYERROR;
+ }
+ iodev = xmalloc(sizeof(struct iodev));
  iodev->path = $2;
  SIMPLEQ_INSERT_TAIL(&domain->iodev_list, iodev, entry);
-    }
+ }
  ;
 
 vnet_opts : { vnet_opts_default(); }

Reply | Threaded
Open this post in threaded view
|

Re: ldomctl: Fail on duplicate vcpu and memory parameters

Klemens Nanni-2
On Mon, Jan 13, 2020 at 12:59:23PM +0100, Klemens Nanni wrote:

> On Sun, Jan 05, 2020 at 07:09:46PM +0100, Klemens Nanni wrote:
> > Domains get to define their cores and memory only once unlike the other
> > parameters of which it makes sense to have more than one.
> >
> > $ cat dup.conf
> > domain primary {
> > vcpu 2
> > vcpu 2
> > }
> > $ ldomctl init-system -n dup.conf ; echo $?
> > 0
> > $ ./obj/ldomctl init-system -n dup.conf
> > dup.conf:3 duplicate vcpu option
> >
> > OK?
> Now that checks for mandatory options are in, this somewhat completes
> it at the other end.
>
> Here's the same diff with an additional check for duplicate iodevices,
> only that those must be globally unique (just like domain names).
>
> OK?
This still is still in my tree.

It properly detects duplicate vcpu and memory lines for all domains
including the primary one;  duplicate iodevice lines are only detected
for guest domains;  preventing iodevice, vnet and vdisk lines in the
primary domain is stuff for a separate diff.

Feedback? OK?


Index: parse.y
===================================================================
RCS file: /cvs/src/usr.sbin/ldomctl/parse.y,v
retrieving revision 1.18
diff -u -p -r1.18 parse.y
--- parse.y 21 Feb 2020 19:39:28 -0000 1.18
+++ parse.y 23 May 2020 09:23:58 -0000
@@ -166,10 +166,18 @@ domainoptsl : domainopts nl
  ;
 
 domainopts : VCPU vcpu {
+ if (domain->vcpu) {
+ yyerror("duplicate vcpu option");
+ YYERROR;
+ }
  domain->vcpu = $2.count;
  domain->vcpu_stride = $2.stride;
  }
  | MEMORY memory {
+ if (domain->memory) {
+ yyerror("duplicate memory option");
+ YYERROR;
+ }
  domain->memory = $2;
  }
  | VDISK STRING vdisk_opts {
@@ -192,10 +200,19 @@ domainopts : VCPU vcpu {
  SIMPLEQ_INSERT_TAIL(&domain->var_list, var, entry);
  }
  | IODEVICE STRING {
- struct iodev *iodev = xmalloc(sizeof(struct iodev));
+ struct domain *odomain;
+ struct iodev *iodev;
+ SIMPLEQ_FOREACH(odomain, &conf->domain_list, entry)
+ SIMPLEQ_FOREACH(iodev, &odomain->iodev_list, entry)
+ if (strcmp(iodev->path, $2) == 0) {
+ yyerror("iodevice assigned"
+    " twice: %s", $2);
+ YYERROR;
+ }
+ iodev = xmalloc(sizeof(struct iodev));
  iodev->path = $2;
  SIMPLEQ_INSERT_TAIL(&domain->iodev_list, iodev, entry);
-    }
+ }
  ;
 
 vdisk_opts : { vdisk_opts_default(); }

Reply | Threaded
Open this post in threaded view
|

Re: ldomctl: Fail on duplicate vcpu and memory parameters

Mark Kettenis
> Date: Sat, 23 May 2020 11:35:46 +0200
> From: Klemens Nanni <[hidden email]>
>
> On Mon, Jan 13, 2020 at 12:59:23PM +0100, Klemens Nanni wrote:
> > On Sun, Jan 05, 2020 at 07:09:46PM +0100, Klemens Nanni wrote:
> > > Domains get to define their cores and memory only once unlike the other
> > > parameters of which it makes sense to have more than one.
> > >
> > > $ cat dup.conf
> > > domain primary {
> > > vcpu 2
> > > vcpu 2
> > > }
> > > $ ldomctl init-system -n dup.conf ; echo $?
> > > 0
> > > $ ./obj/ldomctl init-system -n dup.conf
> > > dup.conf:3 duplicate vcpu option
> > >
> > > OK?
> > Now that checks for mandatory options are in, this somewhat completes
> > it at the other end.
> >
> > Here's the same diff with an additional check for duplicate iodevices,
> > only that those must be globally unique (just like domain names).
> >
> > OK?
> This still is still in my tree.
>
> It properly detects duplicate vcpu and memory lines for all domains
> including the primary one;  duplicate iodevice lines are only detected
> for guest domains;  preventing iodevice, vnet and vdisk lines in the
> primary domain is stuff for a separate diff.
>
> Feedback? OK?

Looks reasonable.  I'd change the message for iodevice though to "iodevice %s already assigned".

ok with that fix.

> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ldomctl/parse.y,v
> retrieving revision 1.18
> diff -u -p -r1.18 parse.y
> --- parse.y 21 Feb 2020 19:39:28 -0000 1.18
> +++ parse.y 23 May 2020 09:23:58 -0000
> @@ -166,10 +166,18 @@ domainoptsl : domainopts nl
>   ;
>  
>  domainopts : VCPU vcpu {
> + if (domain->vcpu) {
> + yyerror("duplicate vcpu option");
> + YYERROR;
> + }
>   domain->vcpu = $2.count;
>   domain->vcpu_stride = $2.stride;
>   }
>   | MEMORY memory {
> + if (domain->memory) {
> + yyerror("duplicate memory option");
> + YYERROR;
> + }
>   domain->memory = $2;
>   }
>   | VDISK STRING vdisk_opts {
> @@ -192,10 +200,19 @@ domainopts : VCPU vcpu {
>   SIMPLEQ_INSERT_TAIL(&domain->var_list, var, entry);
>   }
>   | IODEVICE STRING {
> - struct iodev *iodev = xmalloc(sizeof(struct iodev));
> + struct domain *odomain;
> + struct iodev *iodev;
> + SIMPLEQ_FOREACH(odomain, &conf->domain_list, entry)
> + SIMPLEQ_FOREACH(iodev, &odomain->iodev_list, entry)
> + if (strcmp(iodev->path, $2) == 0) {
> + yyerror("iodevice assigned"
> +    " twice: %s", $2);
> + YYERROR;
> + }
> + iodev = xmalloc(sizeof(struct iodev));
>   iodev->path = $2;
>   SIMPLEQ_INSERT_TAIL(&domain->iodev_list, iodev, entry);
> -    }
> + }
>   ;
>  
>  vdisk_opts : { vdisk_opts_default(); }
>
>

Reply | Threaded
Open this post in threaded view
|

Re: ldomctl: Fail on duplicate vcpu and memory parameters

Klemens Nanni-2
On Sat, May 23, 2020 at 02:38:03PM +0200, Mark Kettenis wrote:
> Looks reasonable.  I'd change the message for iodevice though to "iodevice %s already assigned".
Done, thanks.