[X,B,C,D,unstable] UBUNTU: [Packaging] config-check: Add an include directive

Message ID 20190201180315.28270-1-marcelo.cerri@canonical.com
State New
Headers show
Series
  • [X,B,C,D,unstable] UBUNTU: [Packaging] config-check: Add an include directive
Related show

Commit Message

Marcelo Henrique Cerri Feb. 1, 2019, 6:03 p.m.
BugLink: http://bugs.launchpad.net/bugs/1752072

Update the config-check script to support a new include directive, that can
be used to override annotations from another file. For instance, with
this change a custom kernel can include the annotation file from
"debian.master/" and override some of it policies.

The directive is only available when using the file format 3, that
extends format 2.

The new directive follows the systax:

	include FILEPATH

Quotes are also accepted:

	include "FILEPATH"

`FILENAME` is always relative to the current annotations file location.
So, assuming a custom kernel, the following directive will include the
annotations file from the generic kernel:

	include "../../debian.master/config/annotations"

To avoid mistakes, any reference to a config in the base annotations
file AFTER the include directive will completely override the references
from the included file.

For instance, the following:

    # FORMAT: 3
    include "../../debian.master/config/annotations"
    CONFIG_X note<some note>

Will cause any line related to CONFIG_X in the included annotations file
to be ignored.

The patch also includes smalls changes to avoid warning due to duplicate
variable declarations.

Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
---

That's a patch to replace the patch "config-check: allow overlay annotations files".

---
 debian/scripts/config-check | 80 +++++++++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 21 deletions(-)

Comments

Kamal Mostafa Feb. 5, 2019, 4:54 p.m. | #1
Even better than the first use model -- good stuff.

Acked-by: Kamal Mostafa <kamal@canonical.com>

 -Kamal

On Fri, Feb 01, 2019 at 04:03:15PM -0200, Marcelo Henrique Cerri wrote:
> BugLink: http://bugs.launchpad.net/bugs/1752072
> 
> Update the config-check script to support a new include directive, that can
> be used to override annotations from another file. For instance, with
> this change a custom kernel can include the annotation file from
> "debian.master/" and override some of it policies.
> 
> The directive is only available when using the file format 3, that
> extends format 2.
> 
> The new directive follows the systax:
> 
> 	include FILEPATH
> 
> Quotes are also accepted:
> 
> 	include "FILEPATH"
> 
> `FILENAME` is always relative to the current annotations file location.
> So, assuming a custom kernel, the following directive will include the
> annotations file from the generic kernel:
> 
> 	include "../../debian.master/config/annotations"
> 
> To avoid mistakes, any reference to a config in the base annotations
> file AFTER the include directive will completely override the references
> from the included file.
> 
> For instance, the following:
> 
>     # FORMAT: 3
>     include "../../debian.master/config/annotations"
>     CONFIG_X note<some note>
> 
> Will cause any line related to CONFIG_X in the included annotations file
> to be ignored.
> 
> The patch also includes smalls changes to avoid warning due to duplicate
> variable declarations.
> 
> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> ---
> 
> That's a patch to replace the patch "config-check: allow overlay annotations files".
> 
> ---
>  debian/scripts/config-check | 80 +++++++++++++++++++++++++++----------
>  1 file changed, 59 insertions(+), 21 deletions(-)
> 
> diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> index 224be080514a..843f5999006b 100755
> --- a/debian/scripts/config-check
> +++ b/debian/scripts/config-check
> @@ -3,6 +3,8 @@
>  # check-config -- check the current config for issues
>  #
>  use strict;
> +use File::Basename;
> +use File::Spec;
>  
>  my $P = 'check-config';
>  
> @@ -13,7 +15,7 @@ if ($ARGV[0] eq '--test') {
>  	die "Usage: $P <config> <arch> <flavour> <commonconfig> <warn-only>\n";
>  }
>  
> -my ($config, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
> +my ($configfile, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
>  
>  my %values = ();
>  
> @@ -25,8 +27,8 @@ $fail_exit = 0 if ($warn_only eq 'true' || $warn_only eq '1');
>  my $exit_val = 0;
>  
>  # Load up the current configuration values -- FATAL if this fails
> -print "$P: $config: loading config\n";
> -open(CONFIG, "<$config") || die "$P: $config: open failed -- $! -- aborting\n";
> +print "$P: $configfile: loading config\n";
> +open(CONFIG, "<$configfile") || die "$P: $configfile: open failed -- $! -- aborting\n";
>  while (<CONFIG>) {
>  	# Pull out values.
>  	/^#*\s*(CONFIG_\w+)[\s=](.*)$/ or next;
> @@ -38,38 +40,74 @@ while (<CONFIG>) {
>  }
>  close(CONFIG);
>  
> -# ANNOTATIONS: check any annotations marked for enforcement
> -my $pass = 0;
> -my $total = 0;
> -my $annotations = "$commonconfig/annotations";
> -my ($config, $value, $options, $option, $value, $check, $policy);
> -print "$P: $annotations loading annotations\n";
> -my %annot;
> -my $form = 1;
> -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> -while (<ANNOTATIONS>) {
> +sub read_annotations {
> +    my ($filename) = @_;
> +    my %annot;
> +    my $form = 1;
> +    my ($config, $value, $options);
> +
> +    # Keep track of the configs that shouldn't be appended because
> +    # they were include_annot from another annotations file.
> +    # That's a hash of undefs, aka a set.
> +    my %noappend;
> +
> +    print "$P: $filename loading annotations\n";
> +    open(my $fd, "<$filename") ||
> +	die "$P: $filename: open failed -- $! -- aborting\n";
> +    while (<$fd>) {
>  	if (/^# FORMAT: (\S+)/) {
> -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> -		$form = $1;
> +	    die "$P: $1: unknown annotations format\n" if ($1 != 2 && $1 != 3);
> +	    $form = $1;
> +	}
> +
> +	# Format #3 adds the include directive on top of format #2:
> +	if ($form == 3 && /^\s*include(\s|$)/) {
> +	    # Include quoted or unquoted files:
> +	    if (/^\s*include\s+"(.*)"\s*$/ || /^\s*include\s+(.*)$/) {
> +		# The include is relative to the current file
> +		my $include_filename = File::Spec->join(dirname($filename), $1);
> +		# Append the include files
> +		my %include_annot = read_annotations($include_filename);
> +		%annot = ( %annot, %include_annot );
> +		# And marked them to not be appended:
> +		my %included_noappend;
> +		# Discard the values and keep only the keys
> +		@included_noappend{keys %include_annot} = ();
> +		%noappend = ( %noappend, %included_noappend );
> +		next;
> +	    } else {
> +		die "$P: Invalid include: $_";
> +	    }
>  	}
>  
>  	/^#/ && next;
>  	chomp;
>  	/^$/ && next;
> -
>  	/^CONFIG_/ || next;
>  
>  	if ($form == 1) {
> -		($config, $value, $options) = split(' ', $_, 3);
> -	} elsif ($form == 2) {
> -		($config, $options) = split(' ', $_, 2);
> +	    ($config, $value, $options) = split(' ', $_, 3);
> +	} elsif ($form >= 2) {
> +	    ($config, $options) = split(' ', $_, 2);
>  	}
>  
> +	if (exists $noappend{$config}) {
> +	    delete $annot{$config};
> +	    delete $noappend{$config};
> +	}
>  	$annot{$config} = $annot{$config} . ' ' . $options;
> +    }
> +    close($fd);
> +    return %annot;
>  }
> -close(ANNOTATIONS);
>  
> -my $config;
> +# ANNOTATIONS: check any annotations marked for enforcement
> +my $annotations = "$commonconfig/annotations";
> +my %annot = read_annotations($annotations);
> +
> +my $pass = 0;
> +my $total = 0;
> +my ($config, $value, $options, $option, $check, $policy);
>  for $config (keys %annot) {
>  	$check = 0;
>  	$options = $annot{$config};
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Seth Forshee Feb. 5, 2019, 6:45 p.m. | #2
On Fri, Feb 01, 2019 at 04:03:15PM -0200, Marcelo Henrique Cerri wrote:
> BugLink: http://bugs.launchpad.net/bugs/1752072
> 
> Update the config-check script to support a new include directive, that can
> be used to override annotations from another file. For instance, with
> this change a custom kernel can include the annotation file from
> "debian.master/" and override some of it policies.
> 
> The directive is only available when using the file format 3, that
> extends format 2.
> 
> The new directive follows the systax:
> 
> 	include FILEPATH
> 
> Quotes are also accepted:
> 
> 	include "FILEPATH"
> 
> `FILENAME` is always relative to the current annotations file location.
> So, assuming a custom kernel, the following directive will include the
> annotations file from the generic kernel:
> 
> 	include "../../debian.master/config/annotations"
> 
> To avoid mistakes, any reference to a config in the base annotations
> file AFTER the include directive will completely override the references
> from the included file.
> 
> For instance, the following:
> 
>     # FORMAT: 3
>     include "../../debian.master/config/annotations"
>     CONFIG_X note<some note>
> 
> Will cause any line related to CONFIG_X in the included annotations file
> to be ignored.
> 
> The patch also includes smalls changes to avoid warning due to duplicate
> variable declarations.
> 
> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> ---
> 
> That's a patch to replace the patch "config-check: allow overlay annotations files".
> 
> ---
>  debian/scripts/config-check | 80 +++++++++++++++++++++++++++----------
>  1 file changed, 59 insertions(+), 21 deletions(-)
> 
> diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> index 224be080514a..843f5999006b 100755
> --- a/debian/scripts/config-check
> +++ b/debian/scripts/config-check
> @@ -3,6 +3,8 @@
>  # check-config -- check the current config for issues
>  #
>  use strict;
> +use File::Basename;
> +use File::Spec;
>  
>  my $P = 'check-config';
>  
> @@ -13,7 +15,7 @@ if ($ARGV[0] eq '--test') {
>  	die "Usage: $P <config> <arch> <flavour> <commonconfig> <warn-only>\n";
>  }
>  
> -my ($config, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
> +my ($configfile, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
>  
>  my %values = ();
>  
> @@ -25,8 +27,8 @@ $fail_exit = 0 if ($warn_only eq 'true' || $warn_only eq '1');
>  my $exit_val = 0;
>  
>  # Load up the current configuration values -- FATAL if this fails
> -print "$P: $config: loading config\n";
> -open(CONFIG, "<$config") || die "$P: $config: open failed -- $! -- aborting\n";
> +print "$P: $configfile: loading config\n";
> +open(CONFIG, "<$configfile") || die "$P: $configfile: open failed -- $! -- aborting\n";
>  while (<CONFIG>) {
>  	# Pull out values.
>  	/^#*\s*(CONFIG_\w+)[\s=](.*)$/ or next;
> @@ -38,38 +40,74 @@ while (<CONFIG>) {
>  }
>  close(CONFIG);
>  
> -# ANNOTATIONS: check any annotations marked for enforcement
> -my $pass = 0;
> -my $total = 0;
> -my $annotations = "$commonconfig/annotations";
> -my ($config, $value, $options, $option, $value, $check, $policy);
> -print "$P: $annotations loading annotations\n";
> -my %annot;
> -my $form = 1;
> -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> -while (<ANNOTATIONS>) {
> +sub read_annotations {
> +    my ($filename) = @_;
> +    my %annot;
> +    my $form = 1;
> +    my ($config, $value, $options);
> +
> +    # Keep track of the configs that shouldn't be appended because
> +    # they were include_annot from another annotations file.
> +    # That's a hash of undefs, aka a set.
> +    my %noappend;
> +
> +    print "$P: $filename loading annotations\n";
> +    open(my $fd, "<$filename") ||
> +	die "$P: $filename: open failed -- $! -- aborting\n";
> +    while (<$fd>) {
>  	if (/^# FORMAT: (\S+)/) {
> -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> -		$form = $1;
> +	    die "$P: $1: unknown annotations format\n" if ($1 != 2 && $1 != 3);
> +	    $form = $1;
> +	}
> +
> +	# Format #3 adds the include directive on top of format #2:
> +	if ($form == 3 && /^\s*include(\s|$)/) {
> +	    # Include quoted or unquoted files:
> +	    if (/^\s*include\s+"(.*)"\s*$/ || /^\s*include\s+(.*)$/) {
> +		# The include is relative to the current file
> +		my $include_filename = File::Spec->join(dirname($filename), $1);
> +		# Append the include files
> +		my %include_annot = read_annotations($include_filename);
> +		%annot = ( %annot, %include_annot );
> +		# And marked them to not be appended:
> +		my %included_noappend;
> +		# Discard the values and keep only the keys
> +		@included_noappend{keys %include_annot} = ();
> +		%noappend = ( %noappend, %included_noappend );
> +		next;
> +	    } else {
> +		die "$P: Invalid include: $_";
> +	    }

This handles a definition for a config after an include with that
config. I don't think it handles an include after the definition or two
includes with a duplicate definition, does it?

This may not be a show stopper, and I'm sure it doesn't matter for the
current use case. I just wanted to make sure I understand what works and
what doesn't.

>  	}
>  
>  	/^#/ && next;
>  	chomp;
>  	/^$/ && next;
> -
>  	/^CONFIG_/ || next;
>  
>  	if ($form == 1) {
> -		($config, $value, $options) = split(' ', $_, 3);
> -	} elsif ($form == 2) {
> -		($config, $options) = split(' ', $_, 2);
> +	    ($config, $value, $options) = split(' ', $_, 3);
> +	} elsif ($form >= 2) {
> +	    ($config, $options) = split(' ', $_, 2);
>  	}
>  
> +	if (exists $noappend{$config}) {
> +	    delete $annot{$config};
> +	    delete $noappend{$config};
> +	}
>  	$annot{$config} = $annot{$config} . ' ' . $options;
> +    }
> +    close($fd);
> +    return %annot;
>  }
> -close(ANNOTATIONS);
>  
> -my $config;
> +# ANNOTATIONS: check any annotations marked for enforcement
> +my $annotations = "$commonconfig/annotations";
> +my %annot = read_annotations($annotations);
> +
> +my $pass = 0;
> +my $total = 0;
> +my ($config, $value, $options, $option, $check, $policy);
>  for $config (keys %annot) {
>  	$check = 0;
>  	$options = $annot{$config};
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Marcelo Henrique Cerri Feb. 6, 2019, 11:41 a.m. | #3
On Tue, Feb 05, 2019 at 12:45:23PM -0600, Seth Forshee wrote:
> On Fri, Feb 01, 2019 at 04:03:15PM -0200, Marcelo Henrique Cerri wrote:
> > BugLink: http://bugs.launchpad.net/bugs/1752072
> > 
> > Update the config-check script to support a new include directive, that can
> > be used to override annotations from another file. For instance, with
> > this change a custom kernel can include the annotation file from
> > "debian.master/" and override some of it policies.
> > 
> > The directive is only available when using the file format 3, that
> > extends format 2.
> > 
> > The new directive follows the systax:
> > 
> > 	include FILEPATH
> > 
> > Quotes are also accepted:
> > 
> > 	include "FILEPATH"
> > 
> > `FILENAME` is always relative to the current annotations file location.
> > So, assuming a custom kernel, the following directive will include the
> > annotations file from the generic kernel:
> > 
> > 	include "../../debian.master/config/annotations"
> > 
> > To avoid mistakes, any reference to a config in the base annotations
> > file AFTER the include directive will completely override the references
> > from the included file.
> > 
> > For instance, the following:
> > 
> >     # FORMAT: 3
> >     include "../../debian.master/config/annotations"
> >     CONFIG_X note<some note>
> > 
> > Will cause any line related to CONFIG_X in the included annotations file
> > to be ignored.
> > 
> > The patch also includes smalls changes to avoid warning due to duplicate
> > variable declarations.
> > 
> > Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> > ---
> > 
> > That's a patch to replace the patch "config-check: allow overlay annotations files".
> > 
> > ---
> >  debian/scripts/config-check | 80 +++++++++++++++++++++++++++----------
> >  1 file changed, 59 insertions(+), 21 deletions(-)
> > 
> > diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> > index 224be080514a..843f5999006b 100755
> > --- a/debian/scripts/config-check
> > +++ b/debian/scripts/config-check
> > @@ -3,6 +3,8 @@
> >  # check-config -- check the current config for issues
> >  #
> >  use strict;
> > +use File::Basename;
> > +use File::Spec;
> >  
> >  my $P = 'check-config';
> >  
> > @@ -13,7 +15,7 @@ if ($ARGV[0] eq '--test') {
> >  	die "Usage: $P <config> <arch> <flavour> <commonconfig> <warn-only>\n";
> >  }
> >  
> > -my ($config, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
> > +my ($configfile, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
> >  
> >  my %values = ();
> >  
> > @@ -25,8 +27,8 @@ $fail_exit = 0 if ($warn_only eq 'true' || $warn_only eq '1');
> >  my $exit_val = 0;
> >  
> >  # Load up the current configuration values -- FATAL if this fails
> > -print "$P: $config: loading config\n";
> > -open(CONFIG, "<$config") || die "$P: $config: open failed -- $! -- aborting\n";
> > +print "$P: $configfile: loading config\n";
> > +open(CONFIG, "<$configfile") || die "$P: $configfile: open failed -- $! -- aborting\n";
> >  while (<CONFIG>) {
> >  	# Pull out values.
> >  	/^#*\s*(CONFIG_\w+)[\s=](.*)$/ or next;
> > @@ -38,38 +40,74 @@ while (<CONFIG>) {
> >  }
> >  close(CONFIG);
> >  
> > -# ANNOTATIONS: check any annotations marked for enforcement
> > -my $pass = 0;
> > -my $total = 0;
> > -my $annotations = "$commonconfig/annotations";
> > -my ($config, $value, $options, $option, $value, $check, $policy);
> > -print "$P: $annotations loading annotations\n";
> > -my %annot;
> > -my $form = 1;
> > -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> > -while (<ANNOTATIONS>) {
> > +sub read_annotations {
> > +    my ($filename) = @_;
> > +    my %annot;
> > +    my $form = 1;
> > +    my ($config, $value, $options);
> > +
> > +    # Keep track of the configs that shouldn't be appended because
> > +    # they were include_annot from another annotations file.
> > +    # That's a hash of undefs, aka a set.
> > +    my %noappend;
> > +
> > +    print "$P: $filename loading annotations\n";
> > +    open(my $fd, "<$filename") ||
> > +	die "$P: $filename: open failed -- $! -- aborting\n";
> > +    while (<$fd>) {
> >  	if (/^# FORMAT: (\S+)/) {
> > -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> > -		$form = $1;
> > +	    die "$P: $1: unknown annotations format\n" if ($1 != 2 && $1 != 3);
> > +	    $form = $1;
> > +	}
> > +
> > +	# Format #3 adds the include directive on top of format #2:
> > +	if ($form == 3 && /^\s*include(\s|$)/) {
> > +	    # Include quoted or unquoted files:
> > +	    if (/^\s*include\s+"(.*)"\s*$/ || /^\s*include\s+(.*)$/) {
> > +		# The include is relative to the current file
> > +		my $include_filename = File::Spec->join(dirname($filename), $1);
> > +		# Append the include files
> > +		my %include_annot = read_annotations($include_filename);
> > +		%annot = ( %annot, %include_annot );
> > +		# And marked them to not be appended:
> > +		my %included_noappend;
> > +		# Discard the values and keep only the keys
> > +		@included_noappend{keys %include_annot} = ();
> > +		%noappend = ( %noappend, %included_noappend );
> > +		next;
> > +	    } else {
> > +		die "$P: Invalid include: $_";
> > +	    }
> 
> This handles a definition for a config after an include with that
> config. I don't think it handles an include after the definition or two
> includes with a duplicate definition, does it?

Yes. It's sensitive to the order of definitions and includes. You
could use it, for instance, to provide "default" values (although that
might not be that useful). So in this example:

     # FORMAT: 3
     CONFIG_X policy<{'amd64': 'y'}>
     CONFIG_X mark<ENFORCE> note<reason>
     include "../../debian.master/config/annotations"

If CONFIG_X appears in the included file, everything in the root
annotations file for CONFIG_X will be ignore. And if the included file
does not mention CONFIG_X, 'y' will be enforced for CONFIG_X on amd64.

The same is true for multiple include directives. If more than one
included file defines rules for the same config, the rules in the last
included file will be used, and everything from the previous includes
for that config will be ignored.

I explicitly added the restriction that you can't split the definition
of a single config across several files. For example, you can't have
the mark for a config in one file and the policy in another.

> 
> This may not be a show stopper, and I'm sure it doesn't matter for the
> current use case. I just wanted to make sure I understand what works and
> what doesn't.
>
> >  	}
> >  
> >  	/^#/ && next;
> >  	chomp;
> >  	/^$/ && next;
> > -
> >  	/^CONFIG_/ || next;
> >  
> >  	if ($form == 1) {
> > -		($config, $value, $options) = split(' ', $_, 3);
> > -	} elsif ($form == 2) {
> > -		($config, $options) = split(' ', $_, 2);
> > +	    ($config, $value, $options) = split(' ', $_, 3);
> > +	} elsif ($form >= 2) {
> > +	    ($config, $options) = split(' ', $_, 2);
> >  	}
> >  
> > +	if (exists $noappend{$config}) {
> > +	    delete $annot{$config};
> > +	    delete $noappend{$config};
> > +	}
> >  	$annot{$config} = $annot{$config} . ' ' . $options;
> > +    }
> > +    close($fd);
> > +    return %annot;
> >  }
> > -close(ANNOTATIONS);
> >  
> > -my $config;
> > +# ANNOTATIONS: check any annotations marked for enforcement
> > +my $annotations = "$commonconfig/annotations";
> > +my %annot = read_annotations($annotations);
> > +
> > +my $pass = 0;
> > +my $total = 0;
> > +my ($config, $value, $options, $option, $check, $policy);
> >  for $config (keys %annot) {
> >  	$check = 0;
> >  	$options = $annot{$config};
> > -- 
> > 2.17.1
> > 
> > 
> > -- 
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team


--
Regards,
Marcelo
Seth Forshee Feb. 7, 2019, 5:28 p.m. | #4
On Wed, Feb 06, 2019 at 09:41:51AM -0200, Marcelo Henrique Cerri wrote:
> 
> On Tue, Feb 05, 2019 at 12:45:23PM -0600, Seth Forshee wrote:
> > On Fri, Feb 01, 2019 at 04:03:15PM -0200, Marcelo Henrique Cerri wrote:
> > > BugLink: http://bugs.launchpad.net/bugs/1752072
> > > 
> > > Update the config-check script to support a new include directive, that can
> > > be used to override annotations from another file. For instance, with
> > > this change a custom kernel can include the annotation file from
> > > "debian.master/" and override some of it policies.
> > > 
> > > The directive is only available when using the file format 3, that
> > > extends format 2.
> > > 
> > > The new directive follows the systax:
> > > 
> > > 	include FILEPATH
> > > 
> > > Quotes are also accepted:
> > > 
> > > 	include "FILEPATH"
> > > 
> > > `FILENAME` is always relative to the current annotations file location.
> > > So, assuming a custom kernel, the following directive will include the
> > > annotations file from the generic kernel:
> > > 
> > > 	include "../../debian.master/config/annotations"
> > > 
> > > To avoid mistakes, any reference to a config in the base annotations
> > > file AFTER the include directive will completely override the references
> > > from the included file.
> > > 
> > > For instance, the following:
> > > 
> > >     # FORMAT: 3
> > >     include "../../debian.master/config/annotations"
> > >     CONFIG_X note<some note>
> > > 
> > > Will cause any line related to CONFIG_X in the included annotations file
> > > to be ignored.
> > > 
> > > The patch also includes smalls changes to avoid warning due to duplicate
> > > variable declarations.
> > > 
> > > Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> > > ---
> > > 
> > > That's a patch to replace the patch "config-check: allow overlay annotations files".
> > > 
> > > ---
> > >  debian/scripts/config-check | 80 +++++++++++++++++++++++++++----------
> > >  1 file changed, 59 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> > > index 224be080514a..843f5999006b 100755
> > > --- a/debian/scripts/config-check
> > > +++ b/debian/scripts/config-check
> > > @@ -3,6 +3,8 @@
> > >  # check-config -- check the current config for issues
> > >  #
> > >  use strict;
> > > +use File::Basename;
> > > +use File::Spec;
> > >  
> > >  my $P = 'check-config';
> > >  
> > > @@ -13,7 +15,7 @@ if ($ARGV[0] eq '--test') {
> > >  	die "Usage: $P <config> <arch> <flavour> <commonconfig> <warn-only>\n";
> > >  }
> > >  
> > > -my ($config, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
> > > +my ($configfile, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
> > >  
> > >  my %values = ();
> > >  
> > > @@ -25,8 +27,8 @@ $fail_exit = 0 if ($warn_only eq 'true' || $warn_only eq '1');
> > >  my $exit_val = 0;
> > >  
> > >  # Load up the current configuration values -- FATAL if this fails
> > > -print "$P: $config: loading config\n";
> > > -open(CONFIG, "<$config") || die "$P: $config: open failed -- $! -- aborting\n";
> > > +print "$P: $configfile: loading config\n";
> > > +open(CONFIG, "<$configfile") || die "$P: $configfile: open failed -- $! -- aborting\n";
> > >  while (<CONFIG>) {
> > >  	# Pull out values.
> > >  	/^#*\s*(CONFIG_\w+)[\s=](.*)$/ or next;
> > > @@ -38,38 +40,74 @@ while (<CONFIG>) {
> > >  }
> > >  close(CONFIG);
> > >  
> > > -# ANNOTATIONS: check any annotations marked for enforcement
> > > -my $pass = 0;
> > > -my $total = 0;
> > > -my $annotations = "$commonconfig/annotations";
> > > -my ($config, $value, $options, $option, $value, $check, $policy);
> > > -print "$P: $annotations loading annotations\n";
> > > -my %annot;
> > > -my $form = 1;
> > > -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> > > -while (<ANNOTATIONS>) {
> > > +sub read_annotations {
> > > +    my ($filename) = @_;
> > > +    my %annot;
> > > +    my $form = 1;
> > > +    my ($config, $value, $options);
> > > +
> > > +    # Keep track of the configs that shouldn't be appended because
> > > +    # they were include_annot from another annotations file.
> > > +    # That's a hash of undefs, aka a set.
> > > +    my %noappend;
> > > +
> > > +    print "$P: $filename loading annotations\n";
> > > +    open(my $fd, "<$filename") ||
> > > +	die "$P: $filename: open failed -- $! -- aborting\n";
> > > +    while (<$fd>) {
> > >  	if (/^# FORMAT: (\S+)/) {
> > > -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> > > -		$form = $1;
> > > +	    die "$P: $1: unknown annotations format\n" if ($1 != 2 && $1 != 3);
> > > +	    $form = $1;
> > > +	}
> > > +
> > > +	# Format #3 adds the include directive on top of format #2:
> > > +	if ($form == 3 && /^\s*include(\s|$)/) {
> > > +	    # Include quoted or unquoted files:
> > > +	    if (/^\s*include\s+"(.*)"\s*$/ || /^\s*include\s+(.*)$/) {
> > > +		# The include is relative to the current file
> > > +		my $include_filename = File::Spec->join(dirname($filename), $1);
> > > +		# Append the include files
> > > +		my %include_annot = read_annotations($include_filename);
> > > +		%annot = ( %annot, %include_annot );
> > > +		# And marked them to not be appended:
> > > +		my %included_noappend;
> > > +		# Discard the values and keep only the keys
> > > +		@included_noappend{keys %include_annot} = ();
> > > +		%noappend = ( %noappend, %included_noappend );
> > > +		next;
> > > +	    } else {
> > > +		die "$P: Invalid include: $_";
> > > +	    }
> > 
> > This handles a definition for a config after an include with that
> > config. I don't think it handles an include after the definition or two
> > includes with a duplicate definition, does it?
> 
> Yes. It's sensitive to the order of definitions and includes. You
> could use it, for instance, to provide "default" values (although that
> might not be that useful). So in this example:
> 
>      # FORMAT: 3
>      CONFIG_X policy<{'amd64': 'y'}>
>      CONFIG_X mark<ENFORCE> note<reason>
>      include "../../debian.master/config/annotations"
> 
> If CONFIG_X appears in the included file, everything in the root
> annotations file for CONFIG_X will be ignore. And if the included file
> does not mention CONFIG_X, 'y' will be enforced for CONFIG_X on amd64.
> 
> The same is true for multiple include directives. If more than one
> included file defines rules for the same config, the rules in the last
> included file will be used, and everything from the previous includes
> for that config will be ignored.
> 
> I explicitly added the restriction that you can't split the definition
> of a single config across several files. For example, you can't have
> the mark for a config in one file and the policy in another.

Okay, my perl is weak so I had to go read about hashes a bit to find out
what happens with this line:

  %annot = ( %annot, %include_annot );

if an option already in annot is also in include_annot. It seems the
resulting value for that key will be the one from include_annot, so I
see now that this does work sensibly.

Acked-by: Seth Forshee <seth.forshee@canonical.com>

Applied to disco/master-next and unstable/master, thanks!

> 
> > 
> > This may not be a show stopper, and I'm sure it doesn't matter for the
> > current use case. I just wanted to make sure I understand what works and
> > what doesn't.
> >
> > >  	}
> > >  
> > >  	/^#/ && next;
> > >  	chomp;
> > >  	/^$/ && next;
> > > -
> > >  	/^CONFIG_/ || next;
> > >  
> > >  	if ($form == 1) {
> > > -		($config, $value, $options) = split(' ', $_, 3);
> > > -	} elsif ($form == 2) {
> > > -		($config, $options) = split(' ', $_, 2);
> > > +	    ($config, $value, $options) = split(' ', $_, 3);
> > > +	} elsif ($form >= 2) {
> > > +	    ($config, $options) = split(' ', $_, 2);
> > >  	}
> > >  
> > > +	if (exists $noappend{$config}) {
> > > +	    delete $annot{$config};
> > > +	    delete $noappend{$config};
> > > +	}
> > >  	$annot{$config} = $annot{$config} . ' ' . $options;
> > > +    }
> > > +    close($fd);
> > > +    return %annot;
> > >  }
> > > -close(ANNOTATIONS);
> > >  
> > > -my $config;
> > > +# ANNOTATIONS: check any annotations marked for enforcement
> > > +my $annotations = "$commonconfig/annotations";
> > > +my %annot = read_annotations($annotations);
> > > +
> > > +my $pass = 0;
> > > +my $total = 0;
> > > +my ($config, $value, $options, $option, $check, $policy);
> > >  for $config (keys %annot) {
> > >  	$check = 0;
> > >  	$options = $annot{$config};
> > > -- 
> > > 2.17.1
> > > 
> > > 
> > > -- 
> > > kernel-team mailing list
> > > kernel-team@lists.ubuntu.com
> > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
> 
> 
> --
> Regards,
> Marcelo

Patch

diff --git a/debian/scripts/config-check b/debian/scripts/config-check
index 224be080514a..843f5999006b 100755
--- a/debian/scripts/config-check
+++ b/debian/scripts/config-check
@@ -3,6 +3,8 @@ 
 # check-config -- check the current config for issues
 #
 use strict;
+use File::Basename;
+use File::Spec;
 
 my $P = 'check-config';
 
@@ -13,7 +15,7 @@  if ($ARGV[0] eq '--test') {
 	die "Usage: $P <config> <arch> <flavour> <commonconfig> <warn-only>\n";
 }
 
-my ($config, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
+my ($configfile, $arch, $flavour, $commonconfig, $warn_only) = @ARGV;
 
 my %values = ();
 
@@ -25,8 +27,8 @@  $fail_exit = 0 if ($warn_only eq 'true' || $warn_only eq '1');
 my $exit_val = 0;
 
 # Load up the current configuration values -- FATAL if this fails
-print "$P: $config: loading config\n";
-open(CONFIG, "<$config") || die "$P: $config: open failed -- $! -- aborting\n";
+print "$P: $configfile: loading config\n";
+open(CONFIG, "<$configfile") || die "$P: $configfile: open failed -- $! -- aborting\n";
 while (<CONFIG>) {
 	# Pull out values.
 	/^#*\s*(CONFIG_\w+)[\s=](.*)$/ or next;
@@ -38,38 +40,74 @@  while (<CONFIG>) {
 }
 close(CONFIG);
 
-# ANNOTATIONS: check any annotations marked for enforcement
-my $pass = 0;
-my $total = 0;
-my $annotations = "$commonconfig/annotations";
-my ($config, $value, $options, $option, $value, $check, $policy);
-print "$P: $annotations loading annotations\n";
-my %annot;
-my $form = 1;
-open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
-while (<ANNOTATIONS>) {
+sub read_annotations {
+    my ($filename) = @_;
+    my %annot;
+    my $form = 1;
+    my ($config, $value, $options);
+
+    # Keep track of the configs that shouldn't be appended because
+    # they were include_annot from another annotations file.
+    # That's a hash of undefs, aka a set.
+    my %noappend;
+
+    print "$P: $filename loading annotations\n";
+    open(my $fd, "<$filename") ||
+	die "$P: $filename: open failed -- $! -- aborting\n";
+    while (<$fd>) {
 	if (/^# FORMAT: (\S+)/) {
-		die "$P: $1: unknown annotations format\n" if ($1 != 2);
-		$form = $1;
+	    die "$P: $1: unknown annotations format\n" if ($1 != 2 && $1 != 3);
+	    $form = $1;
+	}
+
+	# Format #3 adds the include directive on top of format #2:
+	if ($form == 3 && /^\s*include(\s|$)/) {
+	    # Include quoted or unquoted files:
+	    if (/^\s*include\s+"(.*)"\s*$/ || /^\s*include\s+(.*)$/) {
+		# The include is relative to the current file
+		my $include_filename = File::Spec->join(dirname($filename), $1);
+		# Append the include files
+		my %include_annot = read_annotations($include_filename);
+		%annot = ( %annot, %include_annot );
+		# And marked them to not be appended:
+		my %included_noappend;
+		# Discard the values and keep only the keys
+		@included_noappend{keys %include_annot} = ();
+		%noappend = ( %noappend, %included_noappend );
+		next;
+	    } else {
+		die "$P: Invalid include: $_";
+	    }
 	}
 
 	/^#/ && next;
 	chomp;
 	/^$/ && next;
-
 	/^CONFIG_/ || next;
 
 	if ($form == 1) {
-		($config, $value, $options) = split(' ', $_, 3);
-	} elsif ($form == 2) {
-		($config, $options) = split(' ', $_, 2);
+	    ($config, $value, $options) = split(' ', $_, 3);
+	} elsif ($form >= 2) {
+	    ($config, $options) = split(' ', $_, 2);
 	}
 
+	if (exists $noappend{$config}) {
+	    delete $annot{$config};
+	    delete $noappend{$config};
+	}
 	$annot{$config} = $annot{$config} . ' ' . $options;
+    }
+    close($fd);
+    return %annot;
 }
-close(ANNOTATIONS);
 
-my $config;
+# ANNOTATIONS: check any annotations marked for enforcement
+my $annotations = "$commonconfig/annotations";
+my %annot = read_annotations($annotations);
+
+my $pass = 0;
+my $total = 0;
+my ($config, $value, $options, $option, $check, $policy);
 for $config (keys %annot) {
 	$check = 0;
 	$options = $annot{$config};