[pve-devel] [PATCH v3 pve-common 0/5] Section Config: Documentation & Code Cleanup

[pve-devel] [PATCH v3 pve-common 0/5] Section Config: Documentation & Code Cleanup

[Max Carrara]

Section Config: Documentation & Code Cleanup - v3
=================================================

Notable Changes Since v2
------------------------

  * Adapt documentation syntax and style
  * Use less emphasis codes `I<>`
  * Add commit to fix spelling

Please see the comments in the individual patches for a detailed list of
changes.

Older Versions
--------------

v2: https://lore.proxmox.com/pve-devel/20240702141314.445130-1-m.carrara@proxmox.com/
v1: https://lore.proxmox.com/pve-devel/20240604092850.126083-1-m.carrara@proxmox.com/

Summary of Changes
------------------

Max Carrara (5):
  section config: document package and its methods with POD
  section config: update code style
  section config: clean up parser logic and semantics
  section config: make subroutine `delete_from_config` private
  section config: fix spelling of variable

 src/PVE/SectionConfig.pm | 1197 ++++++++++++++++++++++++++++++++------
 1 file changed, 1008 insertions(+), 189 deletions(-)

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v3 pve-common 1/5] section config: document package and its methods with POD

[Max Carrara]

Apart from the obvious benefits that documentation has, this also
allows LSPs to provide docstrings e.g. via 'textDocument/hover' [0].

Tested with Perl Navigator [1].

[0]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_hover
[1]: https://github.com/bscan/PerlNavigator

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * update POD style
  * place NAME, DESCRIPTION, USAGE sections above `package` definition
  * remove excess usage of I<> codes
  * remove needless `=pod` paragraphs (e.g. `=head3` alone suffices to
    start POD content)

Changes v1 --> v2:
  * basically all of @Fabian's feedback [0] was taken into account,
    which is way too much to list here, so please see the feedback
    itself
  * use more links where appropriate, e.g. to refer to methods
    associated with a term
  * provide more example code (subroutines `private` and `decode_value`)

[0]: https://lore.proxmox.com/pve-devel/1718097397.xns8i448v3.astroid@yuna.none/

 src/PVE/SectionConfig.pm | 942 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 877 insertions(+), 65 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index a18e9d8..12f7ea4 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1,3 +1,97 @@
+=head1 NAME
+
+C<PVE::SectionConfig> - An Extendible Configuration File Format
+
+=head1 DESCRIPTION
+
+This package provides a way to have multiple (often similar) types of entries
+in the same config file, each in its own section, thus I<Section Config>.
+
+For each C<SectionConfig>-based config file, a C<PVE::JSONSchema> is derived
+automatically. This schema can be used to implement CRUD operations for
+the config data.
+
+The location of a config file is chosen by the author of the code that uses
+C<SectionConfig> and is not something this module is concerned with.
+
+=head1 USAGE
+
+The intended structure is to have a single I<base plugin> that uses the
+C<L<PVE::SectionConfig>> module as a base module. Furthermore, it should provide
+meaningful defaults in its C<$defaultData>, such as a default list of core
+C<L<PVE::JSONSchema>> I<properties>. The I<base plugin> is thus very similar to an
+I<abstract class>.
+
+Each I<child plugin> is then defined in its own package that should inherit
+from the base plugin and defines which properties it itself provides and
+uses, as well as which properties it uses from the base plugin.
+
+The methods that need to be implemented are annotated in the L</METHODS> section
+below.
+
+              ┌─────────────────┐
+              │  SectionConfig  │
+              └────────┬────────┘
+                       │
+                       │
+                       │
+              ┌────────▼────────┐
+              │    BasePlugin   │
+              └────────┬────────┘
+                       │
+             ┌─────────┴─────────┐
+             │                   │
+    ┌────────▼────────┐ ┌────────▼────────┐
+    │ConcretePluginFoo│ │ConcretePluginBar│
+    └─────────────────┘ └─────────────────┘
+
+=head2 REGISTERING PLUGINS
+
+In order to actually be able to use plugins, they must first be
+L<< registered|/$plugin->register() >> and then L<< initialized|/$base->init() >>
+via the I<base plugin>:
+
+    use PVE::Example::BasePlugin;
+    use PVE::Example::PluginA;
+    use PVE::Example::PluginB;
+
+    PVE::Example::PluginA->register();
+    PVE::Example::PluginB->register();
+    PVE::Example::BasePlugin->init();
+
+=head2 MODES
+
+There are two modes for how properties are exposed.
+
+=head3 unified mode (default)
+
+In this mode there is only a global list of properties which the child
+plugins can use. This has the consequence that it's not possible to define the
+same property name more than once in different plugins.
+
+The reason behind this behaviour is to ensure that properties with the same
+name don't behave in different ways, or in other words, to enforce the use of
+identical properties for multiple plugins.
+
+=head3 isolated mode
+
+This mode can be used by calling C<L<< init()|/$base->init() >>> with an additional parameter:
+
+    PVE::Example::BasePlugin->init(property_isolation => 1);
+
+With this mode each I<child plugin> gets its own isolated list of properties,
+or in other words, a fully isolated schema namespace. Normally one wants to use
+C<oneOf> schemas when enabling isolation.
+
+Note that in this mode it's only necessary to specify a property in the
+return value of the C<L<< options()|/options() >>> method when it's either
+C<fixed> or stems from the global list of properties.
+
+All I<locally> defined properties of a child plugin are automatically added to
+its schema.
+
+=cut
+
 package PVE::SectionConfig;
 
 use strict;
@@ -10,65 +104,9 @@ use PVE::Exception qw(raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools;
 
-# This package provides a way to have multiple (often similar) types of entries
-# in the same config file, each in its own section, thus "Section Config".
-#
-# The intended structure is to have a single 'base' plugin that inherits from
-# this class and provides meaningful defaults in its '$defaultData', e.g. a
-# default list of the core properties in its propertyList (most often only 'id'
-# and 'type')
-#
-# Each 'real' plugin then has it's own package that should inherit from the
-# 'base' plugin and returns it's specific properties in the 'properties' method,
-# its type in the 'type' method and all the known options, from both parent and
-# itself, in the 'options' method.
-# The options method can also be used to define if a property is 'optional' or
-# 'fixed' (only settable on config entity-creation), for example:
-#
-# ````
-# sub options {
-#     return {
-#         'some-optional-property' => { optional => 1 },
-#         'a-fixed-property' => { fixed => 1 },
-#         'a-required-but-not-fixed-property' => {},
-#     };
-# }
-# ```
-#
-# 'fixed' options can be set on create, but not changed afterwards.
-#
-# To actually use it, you have to first register all the plugins and then init
-# the 'base' plugin, like so:
-#
-# ```
-# use PVE::Dummy::Plugin1;
-# use PVE::Dummy::Plugin2;
-# use PVE::Dummy::BasePlugin;
-#
-# PVE::Dummy::Plugin1->register();
-# PVE::Dummy::Plugin2->register();
-# PVE::Dummy::BasePlugin->init();
-# ```
-#
-# There are two modes for how properties are exposed, the default 'unified'
-# mode and the 'isolated' mode.
-# In the default unified mode, there is only a global list of properties
-# which the plugins can use, so you cannot define the same property name twice
-# in different plugins. The reason for this is to force the use of identical
-# properties for multiple plugins.
-#
-# The second way is to use the 'isolated' mode, which can be achieved by
-# calling init with `1` as its parameter like this:
-#
-# ```
-# PVE::Dummy::BasePlugin->init(property_isolation => 1);
-# ```
-#
-# With this, each plugin get's their own isolated list of properties which it
-# can use. Note that in this mode, you only have to specify the property in the
-# options method when it is either 'fixed' or comes from the global list of
-# properties. All locally defined ones get automatically added to the schema
-# for that plugin.
+=head2 METHODS
+
+=cut
 
 my $defaultData = {
     options => {},
@@ -77,11 +115,124 @@ my $defaultData = {
     propertyList => {},
 };
 
+=head3 $base->private()
+
+B<REQUIRED:> Must be implemented in the I<base plugin>.
+
+    $data = PVE::Example::Plugin->private()
+    $data = $class->private()
+
+Returns the entire internal state of C<L<PVE::SectionConfig>>, where all plugins
+as well as their C<L<< options()|/$plugin->options() >>> and more are being tracked.
+
+More precisely, this method returns a hash with the following structure:
+
+    {
+	propertyList => {
+	    'some-optional-property' => {
+		type => 'string',
+		optional => 1,
+		description => 'example property',
+	    },
+	    some-property => {
+		description => 'another example property',
+		type => 'boolean'
+	    },
+	},
+	options => {
+	    foo => {
+		'some-optional-property' => { optional => 1 },
+		...
+	    },
+	    ...
+	},
+	plugins => {
+	    foo => 'PVE::Example::FooPlugin',  # reference to package of child plugin
+	    ...
+	},
+	plugindata => {
+	    foo => { ... },  # depends on the specific plugin architecture
+	},
+    }
+
+Where C<foo> is the C<L<< type()|/$plugin->type() >>> of the plugin. See
+C<L<< options()|/$plugin->options() >>> and C<L<< plugindata()|/$plugin->plugindata() >>>
+for more information on their corresponding keys above.
+
+Most commonly this is used to define the default I<property list> of one's
+plugin architecture upfront, for example:
+
+    use PVE::JSONSchema qw(get_standard_option);
+
+    use base qw(PVE::SectionConfig);
+
+    # [...]
+
+    my $defaultData = {
+	propertyList => {
+	    type => {
+		description => "Type of plugin."
+	    },
+	    nodes => get_standard_option('pve-node-list', {
+		description => "List of nodes for which the plugin applies.",
+		optional => 1,
+	    }),
+	    disable => {
+		description => "Flag to disable the plugin.",
+		type => 'boolean',
+		optional => 1,
+	    },
+	    'max-foo-rate' => {
+		description => "Maximum 'foo' rate of the plugin. Use '-1' for unlimited.",
+		type => 'integer',
+		minimum => -1,
+		default => 42,
+		optional => 1,
+	    },
+	    # [...]
+	},
+    };
+
+    sub private {
+	return $defaultData;
+    }
+
+Additional properties defined in I<child plugins> are stored in the
+C<propertyList> key. See C<L<< properties()|/$plugin->properties() >>>.
+
+=cut
+
 sub private {
     die "overwrite me";
     return $defaultData;
 }
 
+=head3 $plugin->register()
+
+    PVE::Example::Plugin->register()
+
+Used to register I<child plugins>.
+
+More specifically, I<registering> a child plugin means that it is added to the
+list of known child plugins that is kept in the hash returned by
+C<L<< private()|/$base->private() >>>. Furthermore, the data returned by
+C<L<< plugindata()|/$plugin->plugindata() >>> is also stored upon registration.
+
+This method must be called on each child plugin before L<< initializing|/$base->init() >>
+the base plugin.
+
+For example:
+
+    use PVE::Example::BasePlugin;
+    use PVE::Example::PluginA;
+    use PVE::Example::PluginB;
+
+    PVE::Example::PluginA->register();
+    PVE::Example::PluginB->register();
+    PVE::Example::BasePlugin->init();
+
+=cut
+
 sub register {
     my ($class) = @_;
 
@@ -96,22 +247,144 @@ sub register {
     $pdata->{plugins}->{$type} = $class;
 }
 
+=head3 $plugin->type()
+
+B<REQUIRED:> Must be implemented in I<B<each>> I<child plugin>.
+
+    $type = PVE::Example::Plugin->type()
+    $type = $class->type()
+
+Returns the I<type> of a child plugin, which is a I<unique> string used to
+identify the child plugin.
+
+Must be overridden on I<B<each>> I<child plugin>, for example:
+
+    sub type {
+	return "foo";
+    }
+
+=cut
+
 sub type {
     die "overwrite me";
 }
 
+=head3 $plugin->properties()
+
+B<OPTIONAL:> Can be overridden in I<child plugins>.
+
+    $props = PVE::Example::Plugin->properties()
+    $props = $class->properties()
+
+Used to register additional properties that belong to a I<child plugin>.
+See below for details on L<the different modes|/MODES>.
+
+This method doesn't need to be overridden if no new properties are necessary.
+
+    sub properties() {
+	return {
+	    path => {
+		description => "Path used to retrieve a 'foo'.",
+		type => 'string',
+		format => 'some-custom-format-handler-for-paths',
+	    },
+	    is_bar = {
+		description => "Whether the 'foo' is 'bar' or not.",
+		type => 'boolean',
+	    },
+	    bwlimit => get_standard_option('bwlimit'),
+	};
+    }
+
+In the default I<L<unified mode|/MODES>>, these properties are added to the
+global list of properties. This means they may also be used by other plugins,
+rather than just by itself. The same property must not be defined by other
+plugins.
+
+In I<L<isolated mode|/MODES>>, these properties are specific to the plugin
+itself and cannot be used by others. They are however automatically added to
+the plugin's schema and made C<optional> by default.
+
+See the C<L<< options()|/$plugin->options() >>> method for more information.
+
+=cut
+
 sub properties {
     return {};
 }
 
+=head3 $plugin->options()
+
+B<OPTIONAL:> Can be overridden in I<child plugins>.
+
+    $opts = PVE::Example::Plugin->options()
+    $opts = $class->options()
+
+This method is used to specify which properties are actually allowed for
+a given I<child plugin>. See below for details on L<the different modes|/MODES>.
+
+Additionally, this method also allows to declare whether a property is
+C<optional> or C<fixed>.
+
+    sub options {
+	return {
+	    'some-optional-property' => { optional => 1 },
+	    'a-fixed-property' => { fixed => 1 },
+	    'a-required-but-not-fixed-property' => {},
+	};
+    }
+
+C<optional> properties are not required to be set.
+
+C<fixed> properties may only be set on creation of the config entity.
+
+In I<L<unified mode|/MODES>> (default), it is necessary to explicitly specify
+which I<properties> are used in the method's return value. Because properties
+are registered globally in this mode, any properties may be specified,
+regardless of which plugin introduced them.
+
+In I<L<isolated mode|/MODES>>, the locally defined properties (those registered
+by overriding C<L<< properties()|/$plugin->properties() >>>) are automatically
+added to the plugin's schema and made C<optional> by default. Should this not be
+desired, a property may still be explicitly defined, in order to make it required
+or C<fixed> instead.
+
+Properties in the global list of properties (see C<L<< private()|/$base->private() >>>)
+are not automatically added and must be explicitly defined instead.
+
+=cut
+
 sub options {
     return {};
 }
 
+=head3 $plugin->plugindata()
+
+B<OPTIONAL:> Can be implemented in I<child plugins>.
+
+    $plugindata = PVE::Example::Plugin->plugindata()
+    $plugindata = $class->plugindata()
+
+This method is used by plugin authors to provide any kind of data specific to
+their plugin implementation and is otherwise not touched by C<L<PVE::SectionConfig>>.
+
+This mostly exists for convenience and doesn't need to be implemented.
+
+=cut
+
 sub plugindata {
     return {};
 }
 
+=head3 $plugin->has_isolated_properties()
+
+    $is_isolated = PVE::Example::Plugin->has_isolated_properties()
+    $is_isolated = $class->has_isolated_properties()
+
+Checks whether the plugin has I<isolated properties> (runs in isolated mode).
+
+=cut
+
 sub has_isolated_properties {
     my ($class) = @_;
 
@@ -168,6 +441,33 @@ my sub add_property {
     }
 };
 
+=head3 $plugin->createSchema()
+
+=head3 $plugin->createSchema([ $skip_type, $base ])
+
+    $schema = PVE::Example::Plugin->($skip_type, $base)
+    $schema = $class->($skip_type, $base)
+
+Returns the C<PVE::JSONSchema> used for I<creating> config entries of a
+I<child plugin>.
+
+This schema may then be used as desired, for example as the definition of
+parameters of an API handler (C<POST>).
+
+=over
+
+=item C<$skip_type> (optional)
+
+Can be set to C<1> to not add the C<type> property to the schema.
+
+=item C<$base> (optional)
+
+The schema of additional properties not derived from the plugin definition.
+
+=back
+
+=cut
+
 sub createSchema {
     my ($class, $skip_type, $base) = @_;
 
@@ -242,6 +542,36 @@ sub createSchema {
     };
 }
 
+=head3 $plugin->updateSchema()
+
+=head3 $plugin->updateSchema([ $single_class, $base ])
+
+    $updated_schema = PVE::Example::Plugin->($single_class, $base)
+    $updated_schema = $class->updateSchema($single_class, $base)
+
+Returns the C<L<PVE::JSONSchema>> used for I<updating> config entries of a
+I<child plugin>.
+
+This schema may then be used as desired, for example as the definition of
+parameters of an API handler (C<PUT>).
+
+=over
+
+=item C<$single_class> (optional)
+
+Can be set to C<1> to only include properties which are defined in the returned
+hash of C<L<< options()|/options() >>> of the plugin C<$class>.
+
+This parameter is only valid for child plugins, not the base plugin.
+
+=item C<$base> (optional)
+
+The schema of additional properties not derived from the plugin definition.
+
+=back
+
+=cut
+
 sub updateSchema {
     my ($class, $single_class, $base) = @_;
 
@@ -326,12 +656,22 @@ sub updateSchema {
     };
 }
 
-# the %param hash controls some behavior of the section config, currently the following options are
-# understood:
-#
-# - property_isolation: if set, each child-plugin has a fully isolated property (schema) namespace.
-#   By default this is off, meaning all child-plugins share the schema of properties with the same
-#   name. Normally one wants to use oneOf schema's when enabling isolation.
+=head3 $base->init()
+
+=head3 $base->init(property_isolation => 1)
+
+    $base_plugin->init();
+    $base_plugin->init(property_isolation => 1);
+
+This method is used to initialize C<SectionConfig> using all of the
+I<child plugins> that were I<L<< registered|/$plugin->register() >>> beforehand.
+
+Optionally, it is also possible to pass C<< property_isolation => 1>> to C<%param>
+in order to activate I<isolated mode>. See L</MODES> in the package-level
+documentation for more information.
+
+=cut
+
 sub init {
     my ($class, %param) = @_;
 
@@ -392,6 +732,16 @@ sub init {
     $propertyList->{type}->{enum} = [sort keys %$plugins];
 }
 
+=head3 $base->lookup($type)
+
+    $plugin = PVE::Example::BasePlugin->lookup($type)
+    $plugin = $class->lookup($type)
+
+Returns the I<child plugin> corresponding to the given C<L<< type()|/$plugin->type() >>>
+or dies if it cannot be found.
+
+=cut
+
 sub lookup {
     my ($class, $type) = @_;
 
@@ -405,6 +755,15 @@ sub lookup {
     return $plugin;
 }
 
+=head3 $base->lookup_types()
+
+    $types = PVE::Example::BasePlugin->lookup_types()
+    $types = $class->lookup_types()
+
+Returns a list of all I<child plugin> C<L<< type|/$plugin->type() >>>s.
+
+=cut
+
 sub lookup_types {
     my ($class) = @_;
 
@@ -413,18 +772,159 @@ sub lookup_types {
     return [ sort keys %{$pdata->{plugins}} ];
 }
 
+=head3 $base->decode_value(...)
+
+=head3 $base->decode_value($type, $key, $value)
+
+B<OPTIONAL:> Can be implemented in the I<base plugin>.
+
+    $decoded_value = PVE::Example::BasePlugin->decode_value($type, $key, $value)
+    $decoded_value = $class->($type, $key, $value)
+
+Called during C<L<< check_config()|/$base->check_config(...) >>> in order to convert values
+that have been read from a C<L<PVE::SectionConfig>> file which have been
+I<encoded> beforehand by C<L<< encode_value()|/$base->encode_value(...) >>>.
+
+Does nothing to C<$value> by default, but can be overridden in the I<base plugin>
+in order to implement custom conversion behavior.
+
+    sub decode_value {
+	my ($class, $type, $key, $value) = @_;
+
+	if ($key eq 'nodes') {
+	    my $res = {};
+
+	    for my $node (PVE::Tools::split_list($value)) {
+		if (PVE::JSONSchema::pve_verify_node_name($node)) {
+		    $res->{$node} = 1;
+		}
+	    }
+
+	    return $res;
+	}
+
+	return $value;
+    }
+
+=over
+
+=item C<$type>
+
+The C<L<< type()|/$plugin->type() >>> of plugin the C<$key> and C<$value> belong to.
+
+=item C<$key>
+
+The name of a I<L<< property|/$plugin->properties() >> that has been set on a C<$type> of
+config section.
+
+=item C<$value>
+
+The raw value of the I<L<< property|/$plugin->properties >>> denoted by C<$key> that was read
+from a section config file.
+
+=back
+
+=cut
+
 sub decode_value {
     my ($class, $type, $key, $value) = @_;
 
     return $value;
 }
 
+=head3 $base->encode_value(...)
+
+=head3 $base->encode_value($type, $key, $value)
+
+B<OPTIONAL:> Can be implemented in the I<base plugin>.
+
+    $encoded_value = PVE::Example::BasePlugin->encode_value($type, $key, $value)
+    $encoded_value = $class->($type, $key, $value)
+
+Called during C<L<< write_config()|/$base->write_config(...) >>> in order to
+convert values into a serializable format.
+
+Does nothing to C<$value> by default, but can be overridden in the I<base plugin>
+in order to implement custom conversion behavior. Usually one should also
+override C<L<< decode_value()|/$base->decode_value(...) >>> in a matching manner.
+
+    sub encode_value {
+	my ($class, $type, $key, $value) = @_;
+
+	if ($key eq 'nodes') {
+	    return join(',', keys(%$value));
+	}
+
+	return $value;
+    }
+
+=over
+
+=item C<$type>
+
+The C<L<< type()|/$plugin->type() >>> of plugin the C<$key> and C<$value> belong to.
+
+=item C<$key>
+
+The name of a I<L<< property|/$plugin->properties() >>> that has been set on a
+C<$type> of config section.
+
+=item C<$value>
+
+The value of the I<L<< property|/$plugin->properties >>> denoted by C<$key> to be
+encoded so that it can be written to a section config file.
+
+=back
+
+=cut
+
 sub encode_value {
     my ($class, $type, $key, $value) = @_;
 
     return $value;
 }
 
+=head3 $base->check_value(...)
+
+=head3 $base->check_value($type, $key, $value, $storeid [, $skipSchemaCheck ])
+
+    $checked_value = PVE::Example::BasePlugin->check_value($type, $key, $value, $storeid, $skipSchemaCheck)
+    $checked_value = $class->check_value($type, $key, $value, $storeid, $skipSchemaCheck)
+
+Used internally to check if various invariants are upheld when parsing a section
+config file. Also performs a C<PVE::JSONSchema> check on the C<$value> of the
+I<property> given by C<$key> of the plugin C<$type>, unless C<$skipSchemaCheck>
+is truthy.
+
+=over
+
+=item C<$type>
+
+The C<L<< type()|/$plugin->type() >>> of plugin the C<$key> and C<$value> belong to.
+
+=item C<$key>
+
+The name of a I<L<< property|/$plugin->properties() >>> that has been set on a
+C<$type> of config section.
+
+=item C<$value>
+
+The value of the I<L<< property|/$plugin->properties() >>> denoted by C<$key>
+that was read from a section config file.
+
+=item C<$storeid>
+
+The identifier of a section, as returned by C<L<< parse_section_header()|/$base->parse_section_header(...) >>>.
+
+=item C<$skipSchemaCheck> (optional)
+
+Whether to skip performing a C<L<PVE::JSONSchema>> property check on the given
+C<$value>.
+
+=back
+
+=cut
+
 sub check_value {
     my ($class, $type, $key, $value, $storeid, $skipSchemaCheck) = @_;
 
@@ -473,6 +973,42 @@ sub check_value {
     return $value;
 }
 
+=head3 $base->parse_section_header($line)
+
+B<OPTIONAL:> Can be overridden in the I<base plugin>.
+
+    ($type, $sectionId, $errmsg, $config) = PVE::Example::BasePlugin->parse_section_header($line)
+    ($type, $sectionId, $errmsg, $config) = $class->parse_section_header($line)
+
+Parses the header of a section and returns an array containing the section's
+L<< type|/$plugin->type() >>, ID and optionally an error message as well as
+additional config attributes.
+
+Can be overridden on the I<base plugin> in order to provide custom logic for
+handling the header, e.g. if the section IDs need to be parsed or validated in
+a certain way.
+
+For example:
+
+    sub parse_section_header {
+	my ($class, $line) = @_;
+
+	if ($line =~ m/^(\S):\s*(\S+)\s*$/) {
+	    my ($type, $sectionId) = ($1, $2);
+
+	    my $errmsg = undef;
+	    eval { check_section_id_is_valid($sectionId); };
+	    $errmsg = $@ if $@;
+
+	    my $config = parse_extra_stuff_from_section_id($sectionId);
+
+	    return ($type, $sectionId, $errmsg, $config);
+	}
+	return undef;
+    }
+
+=cut
+
 sub parse_section_header {
     my ($class, $line) = @_;
 
@@ -485,12 +1021,41 @@ sub parse_section_header {
     return undef;
 }
 
+=head3 $base->format_section_header(...)
+
+=head3 $base->format_section_header($type, $sectionId, $scfg, $done_hash)
+
+B<OPTIONAL:> Can be overridden in the I<base plugin>.
+
+    $header = PVE::Example::BasePlugin->format_section_header($type, $sectionId, $scfg, $done_hash)
+    $header = $class->format_section_header($type, $sectionId, $scfg, $done_hash)
+
+Formats the header of a section. Simply C<"$type: $sectionId\n"> by default.
+
+Note that when overriding this, the header B<MUST> end with a newline (C<\n>).
+One also might want to add a matching override for
+C<L<< parse_section_header()|/$base->parse_section_header($line) >>>.
+
+=cut
+
 sub format_section_header {
     my ($class, $type, $sectionId, $scfg, $done_hash) = @_;
 
     return "$type: $sectionId\n";
 }
 
+=head3 $base->get_property_schema(...)
+
+=head3 $base->get_property_schema($type, $key)
+
+    $schema = PVE::Example::BasePlugin->get_property_schema($type, $key)
+    $schema = $class->get_property_schema($type, $key)
+
+Returns the schema of the L<< property|/$plugin->properties() >> C<$key> of the
+plugin for C<$type>.
+
+=cut
+
 sub get_property_schema {
     my ($class, $type, $key) = @_;
 
@@ -506,6 +1071,109 @@ sub get_property_schema {
     return $schema;
 }
 
+=head3 $base->parse_config(...)
+
+=head3 $base->parse_config($filename, $raw [, $allow_unknown ])
+
+    $config = PVE::Example::BasePlugin->parse_config($filename, $raw, $allow_unknown)
+    $config = $class->parse_config($filename, $raw, $allow_unknown)
+
+Parses the contents of a file as C<L<PVE::SectionConfig>>, returning the parsed
+data annotated with additional information (see below).
+
+=over
+
+=item C<$filename>
+
+The name of the file whose content is stored in C<$raw>.
+
+Only used for error messages and warnings, so it may also be something else.
+
+=item C<$raw>
+
+The raw content of C<$filename>.
+
+=item C<$allow_unknown> (optional)
+
+Whether to allow parsing unknown I<types>.
+
+=back
+
+The returned hash is structured as follows:
+
+    {
+	ids => {
+	    foo => {
+		key => value,
+		...
+	    },
+	    bar => {
+		key => value,
+		...
+	    },
+	},
+	order => {
+	    foo => 1,
+	    bar => 2,
+	},
+	digest => "5f5513f8822fdbe5145af33b64d8d970dcf95c6e",
+	errors => (
+	    {
+		context => ...,
+		section => "section ID",
+		key => "some_key",
+		err => "error message",
+	    },
+	    ...
+	),
+    }
+
+=over
+
+=item C<ids>
+
+Each section's parsed data (via C<L<< check_config()/$base->check_config(...) >>>),
+indexed by the section ID.
+
+=item C<order>
+
+The order in which the sections in C<ids> were found in the config file.
+
+=item C<digest>
+
+A SHA1 hex digest of the contents in C<$raw>. May for example be used to check
+whether the configuration has changed between parses.
+
+=item C<errors> (optional)
+
+An optional list of error hashes.
+
+=back
+
+The hashes in the optionally returned C<errors> key are structured as follows:
+
+=over
+
+=item C<context>
+
+In which file and in which line the error was encountered.
+
+=item C<section>
+
+In which section the error was encountered.
+
+=item C<key>
+
+Which I<property> the error corresponds to.
+
+=item C<err>
+
+The error.
+
+=back
+
+=cut
+
 sub parse_config {
     my ($class, $filename, $raw, $allow_unknown) = @_;
 
@@ -642,6 +1310,60 @@ sub parse_config {
     return $cfg;
 }
 
+=head3 $base->check_config(...)
+
+=head3 $base->check_config($sectionId, $config, $create [, $skipSchemaCheck ])
+
+    $settings = PVE::Example::BasePlugin->check_config($sectionId, $config, $create, $skipSchemaCheck)
+    $settings = $class->check_config($sectionId, $config, $create, $skipSchemaCheck)
+
+Does not just check whether a section's configuration is valid, despite its
+name, but also calls checks values of I<L<< properties|/$plugin_>properties() >>>
+with C<L<< check_value()|/$base->check_value(...) >>> before decoding them using
+C<L<< decode_value()|/$base->decode_value(...) >>>.
+
+Returns a hash which contains all I<L<< properties|/$plugin_>properties() >>>
+for the given C<$sectionId>. In other words, all configured key-value pairs for
+the provided section.
+
+=over
+
+=item C<$sectionId>
+
+The identifier of a section, as returned by C<L<< /$base->parse_section_header($line) >>>.
+
+=item C<$config>
+
+The configuration of the section corresponding to C<$sectionId>.
+
+=item C<$create>
+
+If set to C<1>, checks whether a value has been set for all required properties
+of C<$config>.
+
+=item C<$skipSchemaCheck> (optional)
+
+Whether to skip performing any C<L<PVE::JSONSchema>> property checks.
+
+=back
+
+=head4 A Note on Extending and Overriding
+
+If additional checks are needed that cannot be expressed in the schema, this
+method may be extended or overridden I<with care>.
+
+When this method is I<overridden>, as in the original implementation is replaced
+completely, all values must still be checked via C<L<< check_value()|/$base->check_value(...) >>>
+and decoded with C<L<< decode_value()|/$base->decode_value(...) >>>.
+
+When extending the method, as in calling C<L<< $class->SUPER::check_config()|/$base->check_config(...) >>>
+inside the redefined method, it is important to note that the contents of the
+hash returned by the C<SUPER> call differ from the contents of C<$config>. This
+means that a custom check performed I<before> the C<SUPER> call cannot
+necessarily be performed in the same way I<after> the C<SUPER> call.
+
+=cut
+
 sub check_config {
     my ($class, $sectionId, $config, $create, $skipSchemaCheck) = @_;
 
@@ -700,6 +1422,55 @@ my $format_config_line = sub {
     }
 };
 
+=head3 $base->write_config(...)
+
+=head3 $base->write_config($filename, $cfg [, $allow_unknown ])
+
+    $output = PVE::Example::BasePlugin->write_config($filename, $cfg, $allow_unknown)
+    $output = $class->write_config($filename, $cfg, $allow_unknown)
+
+Generates the output that should be written to the C<L<PVE::SectionConfig>> file.
+
+=over
+
+=item C<$filename> (unused)
+
+The name of the file to which the generated output will be written to.
+This parameter is currently unused and has no effect.
+
+=item C<$cfg>
+
+The hash that represents the entire configuration that should be written.
+This hash is expected to have the following format:
+
+    {
+	ids => {
+	    foo => {
+		key => value,
+		...
+	    },
+	    bar => {
+		key => value,
+		...
+	    },
+	},
+	order => {
+	    foo => 1,
+	    bar => 2,
+	},
+    }
+
+Any other top-level keys will be ignored, so it's okay to pass along the
+C<digest> key from C<L<< parse_config()|/$base->parse_config(...) >>>, for example.
+
+=item C<$allow_unknown> (optional)
+
+Whether to allow writing sections with an unknown I<L</type>>.
+
+=back
+
+=cut
+
 sub write_config {
     my ($class, $filename, $cfg, $allow_unknown) = @_;
 
@@ -798,6 +1569,47 @@ sub assert_if_modified {
     PVE::Tools::assert_if_modified($cfg->{digest}, $digest);
 }
 
+=head3 delete_from_config(...)
+
+=head3 delete_from_config($config, $option_schema, $new_options, $to_delete)
+
+    $config = delete_from_config($config, $option_schema, $new_options, $to_delete)
+
+Convenience helper method used internally to delete keys from the single section
+config C<$config>.
+
+Specifically, the keys given by C<$to_delete> are deleted from C<$config> if
+they're not required or fixed, or set in the same go.
+
+Note: The passed C<$config> is modified in place and also returned.
+
+=over
+
+=item C<$config>
+
+The section's configuration that the given I<L<< properties|/$plugin->properties(...) >>>
+in C<$to_delete> should be deleted from.
+
+=item C<$option_schema>
+
+The schema of the properties associated with C<$config>. See the
+C<L<< options()|/$plugin->options() >>> method.
+
+=item C<$new_options>
+
+The properties which will be added to C<$config>. Note that this method doesn't
+add any properties itself; this is to prohibit simultaneously setting and deleting
+the same I<property>.
+
+=item C<$to_delete>
+
+A reference to an array containing the names of the properties to delete from
+C<$config>.
+
+=back
+
+=cut
+
 sub delete_from_config {
     my ($config, $option_schema, $new_options, $to_delete) = @_;
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v3 pve-common 2/5] section config: update code style

[Max Carrara]

Replace `foreach` with `for` and use postfix deref instead of block
(circumfix) dereference (`$foo->%*` instead of `%$foo`).

Furthermore, make `format_config_line` a private sub instead of
unnecessarily declaring it as an anonymous subroutine, which avoids
the `&$sub_ref(...)` syntax altogether.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * none

Changes v1 --> v2:
  * none

 src/PVE/SectionConfig.pm | 62 ++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 12f7ea4..77d0c17 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -478,7 +478,7 @@ sub createSchema {
     my $props = $base || {};
 
     if (!$class->has_isolated_properties()) {
-	foreach my $p (keys %$propertyList) {
+	for my $p (keys $propertyList->%*) {
 	    next if $skip_type && $p eq 'type';
 
 	    if (!$propertyList->{$p}->{optional}) {
@@ -491,7 +491,7 @@ sub createSchema {
 	    my $copts = $class->options();
 	    $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
 
-	    foreach my $t (keys %$plugins) {
+	    for my $t (keys $plugins->%*) {
 		my $opts = $pdata->{options}->{$t} || {};
 		$required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
 	    }
@@ -506,7 +506,7 @@ sub createSchema {
 	    }
 	}
     } else {
-	for my $type (sort keys %$plugins) {
+	for my $type (sort keys $plugins->%*) {
 	    my $opts = $pdata->{options}->{$type} || {};
 	    for my $key (sort keys $opts->%*) {
 		my $schema = $class->get_property_schema($type, $key);
@@ -584,7 +584,7 @@ sub updateSchema {
     my $filter_type = $single_class ? $class->type() : undef;
 
     if (!$class->has_isolated_properties()) {
-	foreach my $p (keys %$propertyList) {
+	for my $p (keys $propertyList->%*) {
 	    next if $p eq 'type';
 
 	    my $copts = $class->options();
@@ -600,7 +600,7 @@ sub updateSchema {
 
 	    $modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
 
-	    foreach my $t (keys %$plugins) {
+	    for my $t (keys $plugins->%*) {
 		my $opts = $pdata->{options}->{$t} || {};
 		next if !defined($opts->{$p});
 		$modifyable = 1 if !$opts->{$p}->{fixed};
@@ -610,7 +610,7 @@ sub updateSchema {
 	    $props->{$p} = $propertyList->{$p};
 	}
     } else {
-	for my $type (sort keys %$plugins) {
+	for my $type (sort keys $plugins->%*) {
 	    my $opts = $pdata->{options}->{$type} || {};
 	    for my $key (sort keys $opts->%*) {
 		next if $opts->{$key}->{fixed};
@@ -679,7 +679,7 @@ sub init {
 
     my $pdata = $class->private();
 
-    foreach my $k (qw(options plugins plugindata propertyList isolatedPropertyList)) {
+    for my $k (qw(options plugins plugindata propertyList isolatedPropertyList)) {
 	$pdata->{$k} = {} if !$pdata->{$k};
     }
 
@@ -687,9 +687,9 @@ sub init {
     my $propertyList = $pdata->{propertyList};
     my $isolatedPropertyList = $pdata->{isolatedPropertyList};
 
-    foreach my $type (keys %$plugins) {
+    for my $type (keys $plugins->%*) {
 	my $props = $plugins->{$type}->properties();
-	foreach my $p (keys %$props) {
+	for my $p (keys $props->%*) {
 	    my $res;
 	    if ($property_isolation) {
 		$res = $isolatedPropertyList->{$type}->{$p} = {};
@@ -698,16 +698,16 @@ sub init {
 		$res = $propertyList->{$p} = {};
 	    }
 	    my $data = $props->{$p};
-	    for my $a (keys %$data) {
+	    for my $a (keys $data->%*) {
 		$res->{$a} = $data->{$a};
 	    }
 	    $res->{optional} = 1;
 	}
     }
 
-    foreach my $type (keys %$plugins) {
+    for my $type (keys $plugins->%*) {
 	my $opts = $plugins->{$type}->options();
-	foreach my $p (keys %$opts) {
+	for my $p (keys $opts->%*) {
 	    my $prop;
 	    if ($property_isolation) {
 		$prop = $isolatedPropertyList->{$type}->{$p};
@@ -718,7 +718,7 @@ sub init {
 
 	# automatically the properties to options (if not specified explicitly)
 	if ($property_isolation) {
-	    foreach my $p (keys $isolatedPropertyList->{$type}->%*) {
+	    for my $p (keys $isolatedPropertyList->{$type}->%*) {
 		next if $opts->{$p};
 		$opts->{$p} = {};
 		$opts->{$p}->{optional} = 1 if $isolatedPropertyList->{$type}->{$p}->{optional};
@@ -729,7 +729,7 @@ sub init {
     }
 
     $propertyList->{type}->{type} = 'string';
-    $propertyList->{type}->{enum} = [sort keys %$plugins];
+    $propertyList->{type}->{enum} = [sort keys $plugins->%*];
 }
 
 =head3 $base->lookup($type)
@@ -959,7 +959,7 @@ sub check_value {
 	}
 
 	PVE::JSONSchema::check_prop($value, $checkschema, '', $errors);
-	if (scalar(keys %$errors)) {
+	if (scalar(keys $errors->%*)) {
 	    die "$errors->{$key}\n" if $errors->{$key};
 	    die "$errors->{_root}\n" if $errors->{_root};
 	    die "unknown error\n";
@@ -1239,7 +1239,7 @@ sub parse_config {
 	    }
 
 	    while ($line = $nextline->()) {
-		next if $skip; # skip
+		next if $skip;
 
 		$errprefix = "file $filename line $lineno";
 
@@ -1268,7 +1268,7 @@ sub parse_config {
 		    };
 		    if (my $err = $@) {
 			warn "$errprefix (section '$sectionId') - unable to parse value of '$k': $err";
-			push @$errors, {
+			push $errors->@*, {
 			    context => $errprefix,
 			    section => $sectionId,
 			    key => $k,
@@ -1305,7 +1305,7 @@ sub parse_config {
 	order => $order,
 	digest => $digest
     };
-    $cfg->{errors} = $errors if scalar(@$errors) > 0;
+    $cfg->{errors} = $errors if scalar($errors->@*) > 0;
 
     return $cfg;
 }
@@ -1373,7 +1373,7 @@ sub check_config {
 
     my $settings = { type => $type };
 
-    foreach my $k (keys %$config) {
+    for my $k (keys $config->%*) {
 	my $value = $config->{$k};
 
 	die "can't change value of fixed parameter '$k'\n"
@@ -1389,7 +1389,7 @@ sub check_config {
 
     if ($create) {
 	# check if we have a value for all required options
-	foreach my $k (keys %$opts) {
+	for my $k (keys $opts->%*) {
 	    next if $opts->{$k}->{optional};
 	    die "missing value for required option '$k'\n"
 		if !defined($config->{$k});
@@ -1399,7 +1399,7 @@ sub check_config {
     return $settings;
 }
 
-my $format_config_line = sub {
+my sub format_config_line {
     my ($schema, $key, $value) = @_;
 
     my $ct = $schema->{type};
@@ -1482,17 +1482,17 @@ sub write_config {
     my $order = $cfg->{order};
 
     my $maxpri = 0;
-    foreach my $sectionId (keys %$ids) {
+    for my $sectionId (keys $ids->%*) {
 	my $pri = $order->{$sectionId};
 	$maxpri = $pri if $pri && $pri > $maxpri;
     }
-    foreach my $sectionId (keys %$ids) {
+    for my $sectionId (keys $ids->%*) {
 	if (!defined ($order->{$sectionId})) {
 	    $order->{$sectionId} = ++$maxpri;
 	}
     }
 
-    foreach my $sectionId (sort {$order->{$a} <=> $order->{$b}} keys %$ids) {
+    for my $sectionId (sort {$order->{$a} <=> $order->{$b}} keys $ids->%*) {
 	my $scfg = $ids->{$sectionId};
 	my $type = $scfg->{type};
 	my $opts = $pdata->{options}->{$type};
@@ -1507,7 +1507,7 @@ sub write_config {
 	if (!$opts && $allow_unknown) {
 	    $done_hash->{type} = 1;
 	    my @first = exists($scfg->{comment}) ? ('comment') : ();
-	    for my $k (@first, sort keys %$scfg) {
+	    for my $k (@first, sort keys $scfg->%*) {
 		next if defined($done_hash->{$k});
 		$done_hash->{$k} = 1;
 		my $v = $scfg->{$k};
@@ -1527,7 +1527,7 @@ sub write_config {
 	    my $k = 'comment';
 	    my $v = $class->encode_value($type, $k, $scfg->{$k});
 	    my $prop = $class->get_property_schema($type, $k);
-	    $data .= &$format_config_line($prop, $k, $v);
+	    $data .= format_config_line($prop, $k, $v);
 	}
 
 	$data .= "\tdisable\n" if $scfg->{disable} && !$done_hash->{disable};
@@ -1535,8 +1535,8 @@ sub write_config {
 	$done_hash->{comment} = 1;
 	$done_hash->{disable} = 1;
 
-	my @option_keys = sort keys %$opts;
-	foreach my $k (@option_keys) {
+	my @option_keys = sort keys $opts->%*;
+	for my $k (@option_keys) {
 	    next if defined($done_hash->{$k});
 	    next if $opts->{$k}->{optional};
 	    $done_hash->{$k} = 1;
@@ -1545,16 +1545,16 @@ sub write_config {
 		if !defined ($v);
 	    $v = $class->encode_value($type, $k, $v);
 	    my $prop = $class->get_property_schema($type, $k);
-	    $data .= &$format_config_line($prop, $k, $v);
+	    $data .= format_config_line($prop, $k, $v);
 	}
 
-	foreach my $k (@option_keys) {
+	for my $k (@option_keys) {
 	    next if defined($done_hash->{$k});
 	    my $v = $scfg->{$k};
 	    next if !defined($v);
 	    $v = $class->encode_value($type, $k, $v);
 	    my $prop = $class->get_property_schema($type, $k);
-	    $data .= &$format_config_line($prop, $k, $v);
+	    $data .= format_config_line($prop, $k, $v);
 	}
 
 	$out .= "$data\n";
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v3 pve-common 3/5] section config: clean up parser logic and semantics

[Max Carrara]

In order to make the parser somewhat more maintainable in the future,
this commit cleans up its logic and makes its control flow easier to
follow.

Furthermore, regexes are assigned to variables and some variables are
renamed in order to make the code more semantically expressive.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * none

Changes v1 --> v2:
  * reword commit message
  * for errors, use reference of array instead of array directly
  * drop 'x' modifier from $re_kv_pair regex, as it doesn't have any
    benefit

 src/PVE/SectionConfig.pm | 187 ++++++++++++++++++++-------------------
 1 file changed, 97 insertions(+), 90 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 77d0c17..05cd538 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1177,25 +1177,26 @@ The error.
 sub parse_config {
     my ($class, $filename, $raw, $allow_unknown) = @_;
 
-    my $pdata = $class->private();
+    if (!defined($raw)) {
+	return {
+	    ids => {},
+	    order => {},
+	    digest => Digest::SHA::sha1_hex(''),
+	};
+    }
+
+    my $re_begins_with_comment = qr/^\s*#/;
+    my $re_kv_pair = qr/^\s+(\S+)(\s+(.*\S))?\s*$/;
 
     my $ids = {};
     my $order = {};
-
-    $raw = '' if !defined($raw);
-
     my $digest = Digest::SHA::sha1_hex($raw);
 
-    my $pri = 1;
+    my $current_section_no = 1;
+    my $line_no = 0;
 
-    my $lineno = 0;
     my @lines = split(/\n/, $raw);
-    my $nextline = sub {
-	while (defined(my $line = shift @lines)) {
-	    $lineno++;
-	    return $line if ($line !~ /^\s*#/);
-	}
-    };
+    my $errors = [];
 
     my $is_array = sub {
 	my ($type, $key) = @_;
@@ -1206,105 +1207,111 @@ sub parse_config {
 	return $schema->{type} eq 'array';
     };
 
-    my $errors = [];
-    while (@lines) {
-	my $line = $nextline->();
+    my $get_next_line = sub {
+	while (scalar(@lines)) {
+	    my $line = shift(@lines);
+	    $line_no++;
+
+	    next if ($line =~ m/$re_begins_with_comment/);
+
+	    return $line;
+	}
+
+	return undef;
+    };
+
+    my $skip_to_next_empty_line = sub {
+	while ($get_next_line->() ne '') {}
+    };
+
+    while (defined(my $line = $get_next_line->())) {
 	next if !$line;
 
-	my $errprefix = "file $filename line $lineno";
+	my $errprefix = "file $filename line $line_no";
 
-	my ($type, $sectionId, $errmsg, $config) = $class->parse_section_header($line);
-	if ($config) {
-	    my $skip = 0;
-	    my $unknown = 0;
+	my ($type, $section_id, $errmsg, $config) = $class->parse_section_header($line);
 
-	    my $plugin;
+	if (!defined($config)) {
+	    warn "$errprefix - ignore config line: $line\n";
+	    next;
+	}
 
-	    if ($errmsg) {
-		$skip = 1;
-		chomp $errmsg;
-		warn "$errprefix (skip section '$sectionId'): $errmsg\n";
-	    } elsif (!$type) {
-		$skip = 1;
-		warn "$errprefix (skip section '$sectionId'): missing type - internal error\n";
-	    } else {
-		if (!($plugin = $pdata->{plugins}->{$type})) {
-		    if ($allow_unknown) {
-			$unknown = 1;
-		    } else {
-			$skip = 1;
-			warn "$errprefix (skip section '$sectionId'): unsupported type '$type'\n";
-		    }
-		}
-	    }
+	if ($errmsg) {
+	    chomp $errmsg;
+	    warn "$errprefix (skip section '$section_id'): $errmsg\n";
+	    $skip_to_next_empty_line->();
+	    next;
+	}
+
+	if (!$type) {
+	    warn "$errprefix (skip section '$section_id'): missing type - internal error\n";
+	    $skip_to_next_empty_line->();
+	    next;
+	}
+
+	my $plugin = eval { $class->lookup($type) };
+	my $is_unknown_type = defined($@) && $@ ne '';
+
+	if ($is_unknown_type && !$allow_unknown) {
+	    warn "$errprefix (skip section '$section_id'): unsupported type '$type'\n";
+	    $skip_to_next_empty_line->();
+	    next;
+	}
 
-	    while ($line = $nextline->()) {
-		next if $skip;
-
-		$errprefix = "file $filename line $lineno";
-
-		if ($line =~ m/^\s+(\S+)(\s+(.*\S))?\s*$/) {
-		    my ($k, $v) = ($1, $3);
-
-		    eval {
-			if ($unknown) {
-			    if (!defined($config->{$k})) {
-				$config->{$k} = $v;
-			    } else {
-				if (!ref($config->{$k})) {
-				    $config->{$k} = [$config->{$k}];
-				}
-				push $config->{$k}->@*, $v;
-			    }
-			} elsif ($is_array->($type, $k)) {
-			    $v = $plugin->check_value($type, $k, $v, $sectionId);
-			    $config->{$k} = [] if !defined($config->{$k});
-			    push $config->{$k}->@*, $v;
+	# Parse kv-pairs of section - will go on until empty line is encountered
+	while (my $section_line = $get_next_line->()) {
+	    if ($section_line =~ m/$re_kv_pair/) {
+		my ($key, $value) = ($1, $3);
+
+		eval {
+		    if ($is_unknown_type) {
+			if (!defined($config->{$key})) {
+			    $config->{$key} = $value;
 			} else {
-			    die "duplicate attribute\n" if defined($config->{$k});
-			    $v = $plugin->check_value($type, $k, $v, $sectionId);
-			    $config->{$k} = $v;
+			    $config->{$key} = [$config->{$key}] if !ref($config->{$key});
+			    push $config->{$key}->@*, $value;
 			}
-		    };
-		    if (my $err = $@) {
-			warn "$errprefix (section '$sectionId') - unable to parse value of '$k': $err";
-			push $errors->@*, {
-			    context => $errprefix,
-			    section => $sectionId,
-			    key => $k,
-			    err => $err,
-			};
+		    } elsif ($is_array->($type, $key)) {
+			$value = $plugin->check_value($type, $key, $value, $section_id);
+			$config->{$key} = [] if !defined($config->{$key});
+			push $config->{$key}->@*, $value;
+		    } else {
+			die "duplicate attribute\n" if defined($config->{$key});
+			$value = $plugin->check_value($type, $key, $value, $section_id);
+			$config->{$key} = $value;
 		    }
-
-		} else {
-		    warn "$errprefix (section '$sectionId') - ignore config line: $line\n";
+		};
+		if (my $err = $@) {
+		    warn "$errprefix (section '$section_id') - unable to parse value of '$key': $err";
+		    push $errors->@*, {
+			context => $errprefix,
+			section => $section_id,
+			key => $key,
+			err => $err,
+		    };
 		}
 	    }
+	}
 
-	    if ($unknown) {
-		$config->{type} = $type;
-		$ids->{$sectionId} = $config;
-		$order->{$sectionId} = $pri++;
-	    } elsif (!$skip && $type && $plugin && $config) {
-		$config->{type} = $type;
-		if (!$unknown) {
-		    $config = eval { $config = $plugin->check_config($sectionId, $config, 1, 1); };
-		    warn "$errprefix (skip section '$sectionId'): $@" if $@;
-		}
-		$ids->{$sectionId} = $config;
-		$order->{$sectionId} = $pri++;
+	if ($is_unknown_type || ($type && $plugin && $config)) {
+	    $config->{type} = $type;
+
+	    if (!$is_unknown_type) {
+		$config = eval { $config = $plugin->check_config($section_id, $config, 1, 1); };
+		warn "$errprefix (skip section '$section_id'): $@" if $@;
 	    }
 
-	} else {
-	    warn "$errprefix - ignore config line: $line\n";
+	    $ids->{$section_id} = $config;
+	    $order->{$section_id} = $current_section_no++;
 	}
     }
 
     my $cfg = {
 	ids => $ids,
 	order => $order,
-	digest => $digest
+	digest => $digest,
     };
+
     $cfg->{errors} = $errors if scalar($errors->@*) > 0;
 
     return $cfg;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v3 pve-common 4/5] section config: make subroutine `delete_from_config` private

[Max Carrara]

because it's just an internal helper method and isn't used anywhere
outside of the package.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * none

Changes v1 --> v2:
  * new

 src/PVE/SectionConfig.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 05cd538..aa17391 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1617,7 +1617,7 @@ C<$config>.
 
 =cut
 
-sub delete_from_config {
+my sub delete_from_config {
     my ($config, $option_schema, $new_options, $to_delete) = @_;
 
     for my $k ($to_delete->@*) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v3 pve-common 5/5] section config: fix spelling of variable

[Max Carrara]

s/modifyable/modifiable

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new

 src/PVE/SectionConfig.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index aa17391..16ad7aa 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -596,16 +596,16 @@ sub updateSchema {
 		next;
 	    }
 
-	    my $modifyable = 0;
+	    my $modifiable = 0;
 
-	    $modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
+	    $modifiable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
 
 	    for my $t (keys $plugins->%*) {
 		my $opts = $pdata->{options}->{$t} || {};
 		next if !defined($opts->{$p});
-		$modifyable = 1 if !$opts->{$p}->{fixed};
+		$modifiable = 1 if !$opts->{$p}->{fixed};
 	    }
-	    next if !$modifyable;
+	    next if !$modifiable;
 
 	    $props->{$p} = $propertyList->{$p};
 	}
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] partially-applied-series: [PATCH v3 pve-common 0/5] Section Config: Documentation & Code Cleanup

[Thomas Lamprecht]

Am 31.10.24 um 18:07 schrieb Max Carrara:
> Section Config: Documentation & Code Cleanup - v3
> =================================================
> 
> Notable Changes Since v2
> ------------------------
> 
>   * Adapt documentation syntax and style
>   * Use less emphasis codes `I<>`
>   * Add commit to fix spelling
> 
> Please see the comments in the individual patches for a detailed list of
> changes.
> 
> Older Versions
> --------------
> 
> v2: https://lore.proxmox.com/pve-devel/20240702141314.445130-1-m.carrara@proxmox.com/
> v1: https://lore.proxmox.com/pve-devel/20240604092850.126083-1-m.carrara@proxmox.com/
> 
> Summary of Changes
> ------------------
> 
> Max Carrara (5):
>   section config: document package and its methods with POD
>   section config: update code style
>   section config: clean up parser logic and semantics

^- skipped this one for now.

>   section config: make subroutine `delete_from_config` private
>   section config: fix spelling of variable
> 
>  src/PVE/SectionConfig.pm | 1197 ++++++++++++++++++++++++++++++++------
>  1 file changed, 1008 insertions(+), 189 deletions(-)
> 

After talking with Stefan Hanreich off-list he mentioned that this series was
helpfull to some recent section-config related work of his, so partially applied
this series, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 pve-common 3/5] section config: clean up parser logic and semantics

[Thomas Lamprecht]

Am 31.10.24 um 18:07 schrieb Max Carrara:
> In order to make the parser somewhat more maintainable in the future,
> this commit cleans up its logic and makes its control flow easier to
> follow.
> 
> Furthermore, regexes are assigned to variables and some variables are
> renamed in order to make the code more semantically expressive.
> 

Leaving this out for now as this would do well with an in-depth review for
which I do not have the time at the moment, but FWIW any other long-term
staff can do that too. Also, it could be nice to check if there are some
more cases for which new tests could be sensible to add before applying
this.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel