| 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 | expand |
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
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
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
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
I think that patch was left behind on the last sry cycle. That's just a reminder to get it applied. 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 >
Fixed nominations to add cosmic and remove artful On 2019-02-01 16:03:15 , 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
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};
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(-)