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

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

[Max Carrara]

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

Notable Changes Since v1
------------------------

  * Incorporate @Fabian's feedback regarding the docs [0]
  * Reword commit message of patch 03
  * NEW: Patch 04: Make sub `delete_from_config` private

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

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

v1: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064062.html

References
----------

[0]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064165.html

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

Max Carrara (4):
  section config: document package and its methods with POD
  section config: update code style
  section config: clean up parser logic
  section config: make subroutine `delete_from_config` private

 src/PVE/SectionConfig.pm | 1200 ++++++++++++++++++++++++++++++++------
 1 file changed, 1015 insertions(+), 185 deletions(-)

-- 
2.39.2



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

[pve-devel] [PATCH v2 pve-common 1/4] 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 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://lists.proxmox.com/pipermail/pve-devel/2024-June/064165.html

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

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index a18e9d8..f768eb2 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -10,65 +10,103 @@ 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.
+=pod
+
+=head1 NAME
+
+SectionConfig
+
+=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<SectionConfig> module as a base module. Furthermore, it should provide
+meaningful defaults in its C<$defaultData>, such as a default list of core
+C<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 I<base plugin> and defines which I<properties> it itself provides and
+uses, as well as which I<properties> it uses from the I<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
+I<L<registered|/register>> and then I<L<initialized|/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 I<properties> are exposed.
+
+=head3 unified mode (default)
+
+In this mode there is only a global list of I<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>> with an additional parameter:
+
+    PVE::Example::BasePlugin->init(property_isolation => 1);
+
+With this mode each I<child plugin> gets its own isolated list of I<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 I<property> in the
+return value of the C<options> method when it's either C<fixed> or stems from
+the global list of I<properties>.
+
+All locally defined I<properties> of a I<child plugin> are automatically added
+to its schema.
+
+=head2 METHODS
+
+=cut
 
 my $defaultData = {
     options => {},
@@ -77,11 +115,127 @@ my $defaultData = {
     propertyList => {},
 };
 
+=pod
+
+=head3 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<SectionConfig>, where all plugins as well
+as their C<L</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>> of the plugin. See C<L</options>> and
+C<L</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 I<properties> defined in I<child plugins> are stored in the
+C<propertyList> key. See C<L</properties>>.
+
+=cut
+
 sub private {
     die "overwrite me";
     return $defaultData;
 }
 
+=pod
+
+=head3 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>>.
+Furthermore, the data returned by C<L</plugindata>> is also stored upon
+registration.
+
+This method must be called on each child plugin before I<L<initializing|/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 +250,153 @@ sub register {
     $pdata->{plugins}->{$type} = $class;
 }
 
+=pod
+
+=head3 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 I<child plugin>, which is a I<unique> string. This is
+used to identify the I<child plugin>.
+
+Must be overridden on I<B<each>> I<child plugin>, for example:
+
+    sub type {
+	return "foo";
+    }
+
+=cut
+
 sub type {
     die "overwrite me";
 }
 
+=pod
+
+=head3 properties
+
+B<OPTIONAL:> Can be overridden in I<child plugins>.
+
+    $props = PVE::Example::Plugin->properties()
+    $props = $class->properties()
+
+Used to register additional I<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>> method for more information.
+
+=cut
+
 sub properties {
     return {};
 }
 
+=pod
+
+=head3 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 I<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> I<properties> are not required to be set.
+
+C<fixed> I<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>>) 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>>) are not
+automatically added and must be explicitly defined instead.
+
+=cut
+
 sub options {
     return {};
 }
 
+=pod
+
+=head3 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<SectionConfig>.
+
+This mostly exists for convenience and doesn't need to be implemented.
+
+=cut
+
 sub plugindata {
     return {};
 }
 
+=pod
+
+=head3 has_isolated_properties
+
+    $is_isolated = PVE::Example::Plugin->has_isolated_properties()
+    $is_isolated = $class->has_isolated_properties()
+
+Checks whether the plugin has isolated I<properties> (runs in isolated mode).
+
+=cut
+
 sub has_isolated_properties {
     my ($class) = @_;
 
@@ -168,6 +453,33 @@ my sub add_property {
     }
 };
 
+=pod
+
+=head3 createSchema
+
+    $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 +554,36 @@ sub createSchema {
     };
 }
 
+=pod
+
+=head3 updateSchema
+
+    $updated_schema = PVE::Example::Plugin->($single_class, $base)
+    $updated_schema = $class->updateSchema($single_class, $base)
+
+Returns the C<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 I<L</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 +668,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.
+=pod
+
+=head3 init
+
+    $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|/register>> beforehand.
+
+Optionally, it is also possible to pass C<property_isolation =E<gt> 1> as
+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 +744,18 @@ sub init {
     $propertyList->{type}->{enum} = [sort keys %$plugins];
 }
 
+=pod
+
+=head3 lookup
+
+    $plugin = PVE::Example::BasePlugin->lookup($type)
+    $plugin = $class->lookup($type)
+
+Returns the I<child plugin> corresponding to the given I<L</type>> or dies if it
+cannot be found.
+
+=cut
+
 sub lookup {
     my ($class, $type) = @_;
 
@@ -405,6 +769,17 @@ sub lookup {
     return $plugin;
 }
 
+=pod
+
+=head3 lookup_types
+
+    $types = PVE::Example::BasePlugin->lookup_types()
+    $types = $class->lookup_types()
+
+Returns a list of all I<child plugin> C<type>s.
+
+=cut
+
 sub lookup_types {
     my ($class) = @_;
 
@@ -413,18 +788,159 @@ sub lookup_types {
     return [ sort keys %{$pdata->{plugins}} ];
 }
 
+=pod
+
+=head3 decode_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<check_config> in order to convert values that have been read
+from a C<SectionConfig> file which have been I<encoded> beforehand by
+C<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>> of plugin the C<$key> and C<$value> belong to.
+
+=item C<$key>
+
+The name of a I<L<property|/properties>> that has been set on a C<$type> of
+config section.
+
+=item C<$value>
+
+The raw value of the I<L<property|/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;
 }
 
+=pod
+
+=head3 encode_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<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<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>> of plugin the C<$key> and C<$value> belong to.
+
+=item C<$key>
+
+The name of a I<L<property|/properties>> that has been set on a C<$type> of
+config section.
+
+=item C<$value>
+
+The value of the I<L<property|/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;
 }
 
+=pod
+
+=head3 check_value
+
+    $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>> of plugin the C<$key> and C<$value> belong to.
+
+=item C<$key>
+
+The name of a I<L<property|/properties>> that has been set on a C<$type> of
+config section.
+
+=item C<$value>
+
+The value of the I<L<property|/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>>.
+
+=item C<$skipSchemaCheck>
+
+Whether to skip performing a C<PVE::JSONSchema> property check on the given
+C<$value>.
+
+=back
+
+=cut
+
 sub check_value {
     my ($class, $type, $key, $value, $storeid, $skipSchemaCheck) = @_;
 
@@ -473,6 +989,44 @@ sub check_value {
     return $value;
 }
 
+=pod
+
+=head3 parse_section_header
+
+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
+C<type>, ID and optionally an error message as well as additional config
+attributes.
+
+Can be overriden 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 +1039,39 @@ sub parse_section_header {
     return undef;
 }
 
+=pod
+
+=head3 format_section_header
+
+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<parse_section_header>.
+
+=cut
+
 sub format_section_header {
     my ($class, $type, $sectionId, $scfg, $done_hash) = @_;
 
     return "$type: $sectionId\n";
 }
 
+=pod
+
+=head3 get_property_schema
+
+    $schema = PVE::Example::BasePlugin->get_property_schema($type, $key)
+    $schema = $class->get_property_schema($type, $key)
+
+Returns the schema of the L<property|/properties> C<$key> of the plugin for C<$type>.
+
+=cut
+
 sub get_property_schema {
     my ($class, $type, $key) = @_;
 
@@ -506,6 +1087,106 @@ sub get_property_schema {
     return $schema;
 }
 
+=pod
+
+=head3 parse_config
+
+    $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<SectionConfig>, returning the parsed data
+annoted 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>
+
+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>>), 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, where each hash contains the following keys:
+
+=over 2
+
+=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
+
+=back
+
+=cut
+
 sub parse_config {
     my ($class, $filename, $raw, $allow_unknown) = @_;
 
@@ -642,6 +1323,58 @@ sub parse_config {
     return $cfg;
 }
 
+=pod
+
+=head3 check_config
+
+    $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>> with C<L</check_value>>
+before decoding them using C<L</decode_value>>.
+
+Returns a hash which contains all I<L</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</parse_section_header>>.
+
+=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
+I<L</properties>> of C<$config>
+
+=item C<$skipSchemaCheck>
+
+Whether to skip performing any C<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>> and decoded
+with C<L</decode_value>>.
+
+When extending the method, as in calling C<$class-E<gt>SUPER::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 +1433,55 @@ my $format_config_line = sub {
     }
 };
 
+=pod
+
+=head3 write_config
+
+    $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<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>>, for example.
+
+=item C<$allow_unknown>
+
+Whether to allow writing sections with an unknown I<L</type>>.
+
+=back
+
+=cut
+
 sub write_config {
     my ($class, $filename, $cfg, $allow_unknown) = @_;
 
@@ -798,6 +1580,47 @@ sub assert_if_modified {
     PVE::Tools::assert_if_modified($cfg->{digest}, $digest);
 }
 
+=pod
+
+=head3 delete_from_config
+
+    $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>> in C<$to_delete>
+should be deleted from.
+
+=item C<$option_schema>
+
+The schema of the I<L</properties>> associated with C<$config>. See the
+C<L</options>> method.
+
+=item C<$new_options>
+
+The I<L</properties>> which will be added to C<$config>. Note that this method
+doesn't add any I<L</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 I<properties> to delete
+from C<$config>.
+
+=back
+
+=cut
+
 sub delete_from_config {
     my ($config, $option_schema, $new_options, $to_delete) = @_;
 
-- 
2.39.2



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

[pve-devel] [PATCH v2 pve-common 2/4] 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 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 f768eb2..a6cc468 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -490,7 +490,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}) {
@@ -503,7 +503,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};
 	    }
@@ -518,7 +518,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);
@@ -596,7 +596,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();
@@ -612,7 +612,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};
@@ -622,7 +622,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};
@@ -691,7 +691,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};
     }
 
@@ -699,9 +699,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} = {};
@@ -710,16 +710,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};
@@ -730,7 +730,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};
@@ -741,7 +741,7 @@ sub init {
     }
 
     $propertyList->{type}->{type} = 'string';
-    $propertyList->{type}->{enum} = [sort keys %$plugins];
+    $propertyList->{type}->{enum} = [sort keys $plugins->%*];
 }
 
 =pod
@@ -975,7 +975,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";
@@ -1252,7 +1252,7 @@ sub parse_config {
 	    }
 
 	    while ($line = $nextline->()) {
-		next if $skip; # skip
+		next if $skip;
 
 		$errprefix = "file $filename line $lineno";
 
@@ -1281,7 +1281,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,
@@ -1318,7 +1318,7 @@ sub parse_config {
 	order => $order,
 	digest => $digest
     };
-    $cfg->{errors} = $errors if scalar(@$errors) > 0;
+    $cfg->{errors} = $errors if scalar($errors->@*) > 0;
 
     return $cfg;
 }
@@ -1384,7 +1384,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"
@@ -1400,7 +1400,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});
@@ -1410,7 +1410,7 @@ sub check_config {
     return $settings;
 }
 
-my $format_config_line = sub {
+my sub format_config_line {
     my ($schema, $key, $value) = @_;
 
     my $ct = $schema->{type};
@@ -1493,17 +1493,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};
@@ -1518,7 +1518,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};
@@ -1538,7 +1538,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};
@@ -1546,8 +1546,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;
@@ -1556,16 +1556,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.2



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

[pve-devel] [PATCH v2 pve-common 3/4] 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 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 a6cc468..ec074fa 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1190,25 +1190,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) = @_;
@@ -1219,105 +1220,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.2



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

[pve-devel] [PATCH v2 pve-common 4/4] 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 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 ec074fa..77640c9 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1628,7 +1628,7 @@ from 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.2



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

Re: [pve-devel] [PATCH v2 pve-common 0/4] Section Config: Documentation & Code Cleanup

[Max Carrara]

On Tue Jul 2, 2024 at 4:13 PM CEST, Max Carrara wrote:
> Section Config: Documentation & Code Cleanup - v2
> =================================================

Note for reviewers: This series will be superseded in the near future,
so reviewing this isn't necessary at the moment.

>
> Notable Changes Since v1
> ------------------------
>
>   * Incorporate @Fabian's feedback regarding the docs [0]
>   * Reword commit message of patch 03
>   * NEW: Patch 04: Make sub `delete_from_config` private
>
> Please see the comments in the individual patches for a detailed list of
> changes.
>
> Older Versions
> --------------
>
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064062.html
>
> References
> ----------
>
> [0]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064165.html
>
> Summary of Changes
> ------------------
>
> Max Carrara (4):
>   section config: document package and its methods with POD
>   section config: update code style
>   section config: clean up parser logic
>   section config: make subroutine `delete_from_config` private
>
>  src/PVE/SectionConfig.pm | 1200 ++++++++++++++++++++++++++++++++------
>  1 file changed, 1015 insertions(+), 185 deletions(-)



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

Re: [pve-devel] [PATCH v2 pve-common 0/4] Section Config: Documentation & Code Cleanup

[Max Carrara]

On Tue Jul 2, 2024 at 4:13 PM CEST, Max Carrara wrote:
> Section Config: Documentation & Code Cleanup - v2
> =================================================

Superseded by v3:
https://lore.proxmox.com/pve-devel/20241031170720.338794-1-m.carrara@proxmox.com/

>
> Notable Changes Since v1
> ------------------------
>
>   * Incorporate @Fabian's feedback regarding the docs [0]
>   * Reword commit message of patch 03
>   * NEW: Patch 04: Make sub `delete_from_config` private
>
> Please see the comments in the individual patches for a detailed list of
> changes.
>
> Older Versions
> --------------
>
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064062.html
>
> References
> ----------
>
> [0]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064165.html
>
> Summary of Changes
> ------------------
>
> Max Carrara (4):
>   section config: document package and its methods with POD
>   section config: update code style
>   section config: clean up parser logic
>   section config: make subroutine `delete_from_config` private
>
>  src/PVE/SectionConfig.pm | 1200 ++++++++++++++++++++++++++++++++------
>  1 file changed, 1015 insertions(+), 185 deletions(-)



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