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

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

[Max Carrara]

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

The main focus of this series is the comprehensive documentation which
is added in patch 01. While not every single quirk and implementation
detail may be covered, it should be decent enough to help developers
quickly get started with writing their own Section Config plugin
architecture.

The docs are written in POD - not necessarily because it is a convenient
format to write documentation in, but rather because the entire Perl
ecosystem uses it. No need to reinvent the wheel. Furthermore, POD is
supported by LSPs (at least Perl Navigator), which means that the
"docstrings" for Section Config methods will actually show up as inlay
hints.

Patch 02 updates the module's code style to be more in line with our
Perl Style Guide [0].

Patch 03 cleans up the Section Config parser's internal logic and makes
its control flow easier to follow. Note that this patch does *not* mean
to change the parsing behaviour in any way, as that would risk
accidentally breaking existing Section Config plugin architectures or
config APIs.

Testing
-------

Even though the existing unit tests pass, it would be nice if someone
could give this series a spin and tried to change or add various config
settings, e.g. storage settings and SDN settings, to make sure that the
updated parser logic really does behave the same as before.

Generating Docs
---------------

If you prefer to read the added docs in a different format, you may use
`perldoc` to render e.g. a webpage of the module's docs instead:

  $ perldoc -ohtml src/PVE/SectionConfig.pm > docs.html
  $ firefox docs.html

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

Max Carrara (3):
  section config: document package and its methods with POD
  section config: update code style
  section config: clean up parser logic

 src/PVE/SectionConfig.pm | 982 +++++++++++++++++++++++++++++++--------
 1 file changed, 798 insertions(+), 184 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 v1 pve-common 1/3] 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>
---
 src/PVE/SectionConfig.pm | 737 +++++++++++++++++++++++++++++++++++----
 1 file changed, 672 insertions(+), 65 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index a18e9d8..99ee348 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -10,65 +10,102 @@ 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>.
+
+Under the hood, this package automatically creates and manages a matching
+I<JSONSchema> for one's plugin architecture that is used to represent data
+that is read from and written to the config file.
+
+Where this config file is located, as well as its permissions and other related
+things, is up to the plugin author and is not handled by C<PVE::SectionConfig>
+at all.
+
+=head1 USAGE
+
+The intended structure is to have a single I<base plugin> that inherits from
+this class and provides 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<registered>
+and then I<initialized> via the "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<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
+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 +114,85 @@ my $defaultData = {
     propertyList => {},
 };
 
+=pod
+
+=head3 private
+
+B<REQUIRED:> Must be implemented in the I<base plugin>.
+
+    $data = PVE::Example::Plugin->private()
+    $data = $class->private()
+
+Getter for C<SectionConfig>-related private data.
+
+Most commonly this is used to simply retrieve the default I<property> list of
+one's plugin architecture, 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;
+    }
+
+=cut
+
 sub private {
     die "overwrite me";
     return $defaultData;
 }
 
+=pod
+
+=head3 register
+
+    PVE::Example::Plugin->register()
+
+Used to register I<child plugins>.
+
+This method must be called on each child plugin before I<initializing> 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 +207,127 @@ sub register {
     $pdata->{plugins}->{$type} = $class;
 }
 
+=pod
+
+=head3 type
+
+B<REQUIRED:> Must be implemented in I<child plugins>.
+
+    $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>.
+
+Should be overridden on I<child plugins>:
+
+    sub type {
+	return "foo";
+    }
+
+=cut
+
 sub type {
     die "overwrite me";
 }
 
+=pod
+
+=head3 properties
+
+B<REQUIRED:> Must be implemented in I<child plugins>.
+
+    $props = PVE::Example::Plugin->properties()
+    $props = $class->properties()
+
+Returns the I<properties> specific to a I<child plugin> as a hash.
+
+    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'),
+	};
+    }
+
+=cut
+
 sub properties {
     return {};
 }
 
+=pod
+
+=head3 options
+
+B<REQUIRED:> Must be implemented in I<child plugins>.
+
+    $opts = PVE::Example::Plugin->options()
+    $opts = $class->options()
+
+This method is used to specify which I<properties> are actually configured for
+a given I<child plugin>. More precisely, only the I<properties> that are
+contained in the hash this method returns can be used.
+
+Additionally, it 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.
+
+=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 +384,34 @@ 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> instances 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> if there's a I<property> named "type" in the list of
+default I<properties> that should be excluded from the generated schema.
+
+=item C<$base> (optional)
+
+The I<properties> to use per default.
+
+=back
+
+=cut
+
 sub createSchema {
     my ($class, $skip_type, $base) = @_;
 
@@ -242,6 +486,18 @@ sub createSchema {
     };
 }
 
+=pod
+
+=head3 updateSchema
+
+Returns the C<PVE::JSONSchema> used for I<updating> instances 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>).
+
+=cut
+
 sub updateSchema {
     my ($class, $single_class, $base) = @_;
 
@@ -326,12 +582,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 all I<child plugins> that have been
+I<registered> beforehand.
+
+Optionally, it is also possible to pass C<property_isolation> as parameter 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 +658,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 C<type> or dies if it
+cannot be found.
+
+=cut
+
 sub lookup {
     my ($class, $type) = @_;
 
@@ -405,6 +683,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 plugins'> C<type>s.
+
+=cut
+
 sub lookup_types {
     my ($class) = @_;
 
@@ -413,18 +702,66 @@ 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.
+
+=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.
+
+=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. It's best to not
+override this.
+
+=cut
+
 sub check_value {
     my ($class, $type, $key, $value, $storeid, $skipSchemaCheck) = @_;
 
@@ -473,6 +810,46 @@ sub check_value {
     return $value;
 }
 
+=pod
+
+=head3 parse_section_header
+
+B<OPTIONAL:> Can be I<extended> 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.
+
+Note that the section B<MUST> initially be parsed with the regex used by the
+original method when overriding in order to guarantee compatibility.
+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 +862,40 @@ 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 a I<property> of a I<child plugin> that is denoted via
+its C<$type>.
+
+=cut
+
 sub get_property_schema {
     my ($class, $type, $key) = @_;
 
@@ -506,6 +911,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 C<SectionConfig> file and returns a complex nested
+hash which not only contains the parsed data, but additional information that
+one may or may not find useful. More below.
+
+=over
+
+=item C<$filename>
+
+The name of the file whose content is stored in C<$raw>.
+
+=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 configuration values, or more precisely, the I<section
+identifiers> and their associated configuration options as returned by
+C<check_config>.
+
+=item C<order>
+
+The order in which the sections in C<ids> were parsed.
+
+=item C<digest>
+
+A SHA1 hex digest of the contents in C<$raw>.
+
+=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 +1147,23 @@ 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 C<decode_value> (among other things) internally.
+
+Returns a hash which contains all I<properties> for the given C<$sectionId>.
+In other words, all configured key-value pairs for the provided section.
+
+It's best to not override this.
+
+=cut
+
 sub check_config {
     my ($class, $sectionId, $config, $create, $skipSchemaCheck) = @_;
 
@@ -700,6 +1222,52 @@ 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,
+	},
+    }
+
+=item C<$allow_unknown>
+
+Whether to allow writing sections with an unknown C<type>.
+
+=back
+
+=cut
+
 sub write_config {
     my ($class, $filename, $cfg, $allow_unknown) = @_;
 
@@ -798,6 +1366,45 @@ sub assert_if_modified {
     PVE::Tools::assert_if_modified($cfg->{digest}, $digest);
 }
 
+=pod
+
+=head3 delete_from_config
+
+    $config = PVE::Example::BasePlugin->delete_from_config($config, $option_schema, $new_options, $to_delete)
+    $config = $class->delete_from_config($config, $option_schema, $new_options, $to_delete)
+
+Convenience method to delete key from a hash of configured I<properties> which
+performs necessary checks beforehand.
+
+Note: The passed C<$config> is modified in place and also returned.
+
+=over
+
+=item C<$config>
+
+The section's configuration that the given I<properties> in C<$to_delete> should
+be deleted from.
+
+=item C<$option_schema>
+
+The schema of the I<properties> associated with C<$config>. See the C<options>
+method.
+
+=item C<$new_options>
+
+The I<properties> which are to be added to C<$config>. Note that this method
+doesn't add any I<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

Re: [pve-devel] [PATCH v1 pve-common 1/3] section config: document package and its methods with POD

[Fabian Grünbichler]

On June 4, 2024 11:28 am, Max Carrara wrote:
> 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>
> ---
>  src/PVE/SectionConfig.pm | 737 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 672 insertions(+), 65 deletions(-)
> 
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index a18e9d8..99ee348 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -10,65 +10,102 @@ 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>.
> +
> +Under the hood, this package automatically creates and manages a matching
> +I<JSONSchema> for one's plugin architecture that is used to represent data
> +that is read from and written to the config file.

this sentence is too long and hard to parse.

For each SectionConfig-based config file, a I<JSONSchema> is derived
automatically. This schema can be used to implement CRUD operations for
the config data.

or something like that?

> +
> +Where this config file is located, as well as its permissions and other related
> +things, is up to the plugin author and is not handled by C<PVE::SectionConfig>
> +at all.

it's not really up to the plugin author? it's up to the author of the
code using SectionConfig, which might be someone else entirely. the
plugin author might be some third party..

> +
> +=head1 USAGE
> +
> +The intended structure is to have a single I<base plugin> that inherits from
> +this class and provides meaningful defaults in its C<$defaultData>, such as a

s/inherits from this class/uses the SectionConfig module as base module

in Perl, a class is a perl structure that has been blessed. while the
code here uses '$class', it's not really one.

> +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│
> +    └─────────────────┘ └─────────────────┘

lots of trailing whitespace there..

                              /-> ConcretePluginFoo
SectionConfig -> BasePlugin -X
                              \-> ConcretePluginBar

would show the same info IMHO, but I don't care much either way :-P

> +
> +=head2 REGISTERING PLUGINS
> +
> +In order to actually be able to use plugins, they must first be I<registered>
> +and then I<initialized> via the "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<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
> +C<options> method when it's either C<fixed> or stems from the global list of

in the *return value of the*

> +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 +114,85 @@ my $defaultData = {
>      propertyList => {},
>  };
>  
> +=pod
> +
> +=head3 private
> +
> +B<REQUIRED:> Must be implemented in the I<base plugin>.
> +
> +    $data = PVE::Example::Plugin->private()
> +    $data = $class->private()
> +
> +Getter for C<SectionConfig>-related private data.
> +
> +Most commonly this is used to simply retrieve the default I<property> list of
> +one's plugin architecture, 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;
> +    }
> +
> +=cut
> +

this is not really explaining what this sub does.. while yes, it's
implemented in the base plugin and returns the default data there, this
is actually the getter for the internal state of the whole
SectionConfig..

>  sub private {
>      die "overwrite me";
>      return $defaultData;
>  }
>  
> +=pod
> +
> +=head3 register
> +
> +    PVE::Example::Plugin->register()
> +
> +Used to register I<child plugins>.
> +
> +This method must be called on each child plugin before I<initializing> the base
> +plugin.
> +

and what does it do? :)

> +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 +207,127 @@ sub register {
>      $pdata->{plugins}->{$type} = $class;
>  }
>  
> +=pod
> +
> +=head3 type
> +
> +B<REQUIRED:> Must be implemented in I<child plugins>.

on fact, this must be implemented in *each* child plugin (a difference
that matters ;))

> +
> +    $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>.
> +
> +Should be overridden on I<child plugins>:

no, it must ;)

> +
> +    sub type {
> +	return "foo";
> +    }
> +
> +=cut
> +
>  sub type {
>      die "overwrite me";
>  }
>  
> +=pod
> +
> +=head3 properties
> +
> +B<REQUIRED:> Must be implemented in I<child plugins>.

not technically true (or rather, always true)? it is implemented in all
plugins since it is implemented below.. and it's fine to not override it
in a specific plugin if that is not necessary..

> +
> +    $props = PVE::Example::Plugin->properties()
> +    $props = $class->properties()
> +
> +Returns the I<properties> specific to a I<child plugin> as a hash.

this also doesn't really explain what this is used for IMHO.. this is
used to *register* properties for use in this or other plugin's
`options()` return value. depending on the isolation mode, of course.

> +
> +    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'),
> +	};
> +    }
> +
> +=cut
> +
>  sub properties {
>      return {};
>  }
>  
> +=pod
> +
> +=head3 options
> +
> +B<REQUIRED:> Must be implemented in I<child plugins>.

again, this is not really true.. especially in the case of property
isolation, it's fine to leave this blank, since the properties are used
to initialize the options..

> +
> +    $opts = PVE::Example::Plugin->options()
> +    $opts = $class->options()
> +
> +This method is used to specify which I<properties> are actually configured for

configured? allowed/used fits better IMHO

> +a given I<child plugin>. More precisely, only the I<properties> that are
> +contained in the hash this method returns can be used.

wrong, see above..

> +
> +Additionally, it also allows to declare whether a property is C<optional> or
> +C<fixed>.

optional can also be set via the property itself, in case of isolated
properties.

> +
> +    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.
> +
> +=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 +384,34 @@ 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> instances of a
> +I<child plugin>.

"instances of a child plugin" sounds confusing. config entries defined
by a 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> if there's a I<property> named "type" in the list of
> +default I<properties> that should be excluded from the generated schema.

can be described in much simpler terms:

Set to '1' to not add 'type' property to schema.

> +
> +=item C<$base> (optional)
> +
> +The I<properties> to use per default.

no, this defines the schema of additional properties not derived from
the plugin definition.

> +
> +=back
> +
> +=cut
> +
>  sub createSchema {
>      my ($class, $skip_type, $base) = @_;
>  
> @@ -242,6 +486,18 @@ sub createSchema {
>      };
>  }
>  
> +=pod
> +
> +=head3 updateSchema
> +
> +Returns the C<PVE::JSONSchema> used for I<updating> instances of a
> +I<child plugin>.

same as above..

> +
> +This schema may then be used as desired, for example as the definition of
> +parameters of an API handler (C<PUT>).
> +
> +=cut

what about the parameters here? ;)

> +
>  sub updateSchema {
>      my ($class, $single_class, $base) = @_;
>  
> @@ -326,12 +582,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 all I<child plugins> that have been
> +I<registered> beforehand.

not really.. it is used to initialize the SectionConfig itself using the
registered plugins..

> +
> +Optionally, it is also possible to pass C<property_isolation> as parameter in
> +order to activate I<isolated mode>. See L</MODES> in the package-level
> +documentation for more information.

this actually is worse then the old variant since the information *how
to set* it is lost here..

> +
> +=cut
> +
>  sub init {
>      my ($class, %param) = @_;
>  
> @@ -392,6 +658,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 C<type> or dies if it
> +cannot be found.
> +
> +=cut
> +
>  sub lookup {
>      my ($class, $type) = @_;
>  
> @@ -405,6 +683,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 plugins'> C<type>s.

I<child plugin> C<type>s ?

> +
> +=cut
> +
>  sub lookup_types {
>      my ($class) = @_;
>  
> @@ -413,18 +702,66 @@ 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.
> +
> +=cut

description what type/key/value are would be nice? also, some example?

> +
>  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.
> +
> +=cut

same here

> +
>  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. It's best to not
> +override this.

should we make it internal then?

> +
> +=cut
> +
>  sub check_value {
>      my ($class, $type, $key, $value, $storeid, $skipSchemaCheck) = @_;
>  
> @@ -473,6 +810,46 @@ sub check_value {
>      return $value;
>  }
>  
> +=pod
> +
> +=head3 parse_section_header
> +
> +B<OPTIONAL:> Can be I<extended> 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.
> +
> +Note that the section B<MUST> initially be parsed with the regex used by the
> +original method when overriding in order to guarantee compatibility.

why? that's not true at all, and also not true for our code using this
(e.g., see PVE::Mapping::USB, where the type is encoded in the file
name, and the section header just consists of the id followed by a
newline).

> +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 +862,40 @@ 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 a I<property> of a I<child plugin> that is denoted via
> +its C<$type>.

Returns the schema of the property '$key' of the plugin for '$type'.

> +
> +=cut
> +
>  sub get_property_schema {
>      my ($class, $type, $key) = @_;
>  
> @@ -506,6 +911,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 C<SectionConfig> file and returns a complex nested
> +hash which not only contains the parsed data, but additional information that
> +one may or may not find useful. More below.

that comment about usefulness serves no purpose.

Parses the contents of a file as 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>.

this is just used for error messages/warnings, so it can 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 configuration values, or more precisely, the I<section
> +identifiers> and their associated configuration options as returned by
> +C<check_config>.

Each section's parsed data (via check_config), indexed by the section
ID.

> +
> +=item C<order>
> +
> +The order in which the sections in C<ids> were parsed.

not only parsed, but found in the config file.

> +
> +=item C<digest>
> +
> +A SHA1 hex digest of the contents in C<$raw>.

the purpose would be good to add here ;)

> +
> +=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 +1147,23 @@ 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 C<decode_value> (among other things) internally.
> +
> +Returns a hash which contains all I<properties> for the given C<$sectionId>.
> +In other words, all configured key-value pairs for the provided section.
> +
> +It's best to not override this.

that's not true, this is actually okay to override if you need
additional checks that can't be expressed in the schema. you do have to
be aware of the decode_value part.. some of that can also be expressed
via hooks though.

> +
> +=cut
> +
>  sub check_config {
>      my ($class, $sectionId, $config, $create, $skipSchemaCheck) = @_;
>  
> @@ -700,6 +1222,52 @@ 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 (it's okay to pass in `digest`
as well, for example).

> +
> +=item C<$allow_unknown>
> +
> +Whether to allow writing sections with an unknown C<type>.
> +
> +=back
> +
> +=cut
> +
>  sub write_config {
>      my ($class, $filename, $cfg, $allow_unknown) = @_;
>  
> @@ -798,6 +1366,45 @@ sub assert_if_modified {
>      PVE::Tools::assert_if_modified($cfg->{digest}, $digest);
>  }
>  
> +=pod
> +
> +=head3 delete_from_config
> +
> +    $config = PVE::Example::BasePlugin->delete_from_config($config, $option_schema, $new_options, $to_delete)
> +    $config = $class->delete_from_config($config, $option_schema, $new_options, $to_delete)
> +
> +Convenience method to delete key from a hash of configured I<properties> which
> +performs necessary checks beforehand.

this whole part here is very confusing, and also, this is not even part
of the plugin interface at all, it's just a convenience helper operating
on a single section's config data..

what it does is delete from $config if allowed (not required or fixed or
set it 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<properties> in C<$to_delete> should
> +be deleted from.
> +
> +=item C<$option_schema>
> +
> +The schema of the I<properties> associated with C<$config>. See the C<options>
> +method.
> +
> +=item C<$new_options>
> +
> +The I<properties> which are to be added to C<$config>. Note that this method
> +doesn't add any I<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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-common 1/3] section config: document package and its methods with POD

[Max Carrara]

On Tue Jun 11, 2024 at 12:46 PM CEST, Fabian Grünbichler wrote:
> On June 4, 2024 11:28 am, Max Carrara wrote:
> > 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>
> > ---

Making a cut here because I agree with pretty much everything you said;
will incorporate all of your comments into v2 (unless some questions
arise from my side). Thanks a lot for your thorough review, it's
really appreciated! :)

---- snip 8<------



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

[pve-devel] [PATCH v1 pve-common 2/3] 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>
---
 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 99ee348..a6b0183 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -422,7 +422,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}) {
@@ -435,7 +435,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};
 	    }
@@ -450,7 +450,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);
@@ -510,7 +510,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();
@@ -526,7 +526,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};
@@ -536,7 +536,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};
@@ -605,7 +605,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};
     }
 
@@ -613,9 +613,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} = {};
@@ -624,16 +624,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};
@@ -644,7 +644,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};
@@ -655,7 +655,7 @@ sub init {
     }
 
     $propertyList->{type}->{type} = 'string';
-    $propertyList->{type}->{enum} = [sort keys %$plugins];
+    $propertyList->{type}->{enum} = [sort keys $plugins->%*];
 }
 
 =pod
@@ -796,7 +796,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";
@@ -1076,7 +1076,7 @@ sub parse_config {
 	    }
 
 	    while ($line = $nextline->()) {
-		next if $skip; # skip
+		next if $skip;
 
 		$errprefix = "file $filename line $lineno";
 
@@ -1105,7 +1105,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,
@@ -1142,7 +1142,7 @@ sub parse_config {
 	order => $order,
 	digest => $digest
     };
-    $cfg->{errors} = $errors if scalar(@$errors) > 0;
+    $cfg->{errors} = $errors if scalar($errors->@*) > 0;
 
     return $cfg;
 }
@@ -1173,7 +1173,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"
@@ -1189,7 +1189,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});
@@ -1199,7 +1199,7 @@ sub check_config {
     return $settings;
 }
 
-my $format_config_line = sub {
+my sub format_config_line {
     my ($schema, $key, $value) = @_;
 
     my $ct = $schema->{type};
@@ -1279,17 +1279,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};
@@ -1304,7 +1304,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};
@@ -1324,7 +1324,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};
@@ -1332,8 +1332,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;
@@ -1342,16 +1342,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 v1 pve-common 3/3] section config: clean up parser logic

[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.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/SectionConfig.pm | 189 ++++++++++++++++++++-------------------
 1 file changed, 98 insertions(+), 91 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index a6b0183..30faaa4 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1014,25 +1014,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*$/x;
 
     my $ids = {};
     my $order = {};
-
-    $raw = '' if !defined($raw);
-
     my $digest = Digest::SHA::sha1_hex($raw);
 
-    my $pri = 1;
+    my $current_order = 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) = @_;
@@ -1043,106 +1044,112 @@ 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;
+	}
+
+	# 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);
 
-	    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;
+		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_order++;
 	}
     }
 
     my $cfg = {
 	ids => $ids,
 	order => $order,
-	digest => $digest
+	digest => $digest,
     };
-    $cfg->{errors} = $errors if scalar($errors->@*) > 0;
+
+    $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

Re: [pve-devel] [PATCH v1 pve-common 3/3] section config: clean up parser logic

[Fabian Grünbichler]

On June 4, 2024 11:28 am, Max Carrara wrote:
> 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.

it also mixes deeper changes with things like variable renaming, which
makes it unnecessarily hard to review..

> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/SectionConfig.pm | 189 ++++++++++++++++++++-------------------
>  1 file changed, 98 insertions(+), 91 deletions(-)
> 
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index a6b0183..30faaa4 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -1014,25 +1014,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*$/x;

I am not sure this is really more readable? especially in a RE that is
basically only concerned with whitespace.. and for a RE that is only
used once..

>  
>      my $ids = {};
>      my $order = {};
> -
> -    $raw = '' if !defined($raw);
> -
>      my $digest = Digest::SHA::sha1_hex($raw);
>  
> -    my $pri = 1;
> +    my $current_order = 1;

this is actually a worse name than before. this would be something like
current_index? current_position? section_no?

> +    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;

what do we gain from this?

>  
>      my $is_array = sub {
>  	my ($type, $key) = @_;
> @@ -1043,106 +1044,112 @@ 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;
> +	}
> +
> +	# 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);
>  
> -	    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;
> +		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_order++;
>  	}
>      }
>  
>      my $cfg = {
>  	ids => $ids,
>  	order => $order,
> -	digest => $digest
> +	digest => $digest,
>      };
> -    $cfg->{errors} = $errors if scalar($errors->@*) > 0;
> +
> +    $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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-common 3/3] section config: clean up parser logic

[Max Carrara]

On Tue Jun 11, 2024 at 12:46 PM CEST, Fabian Grünbichler wrote:
> On June 4, 2024 11:28 am, Max Carrara wrote:
> > 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.
>
> it also mixes deeper changes with things like variable renaming, which
> makes it unnecessarily hard to review..

Should've mentioned that; mea culpa. Will see if I can retain the old
names, otherwise will mention / list the renamed variables.

>
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> >  src/PVE/SectionConfig.pm | 189 ++++++++++++++++++++-------------------
> >  1 file changed, 98 insertions(+), 91 deletions(-)
> > 
> > diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> > index a6b0183..30faaa4 100644
> > --- a/src/PVE/SectionConfig.pm
> > +++ b/src/PVE/SectionConfig.pm
> > @@ -1014,25 +1014,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*$/x;
>
> I am not sure this is really more readable? especially in a RE that is
> basically only concerned with whitespace.. and for a RE that is only
> used once..

I mean I could drop the `x` flag if you'd like, just thought that it
would make it a little easier to parse... Though, the reason why I'm
personally a fan of assigning regexes to variables is because it makes
the code more expressive IMO. If someone who usually doesn't deal with
regexes has to read this code, the variable name describes what the
regex does. That's mainly the reason why I put it there.

>
> >  
> >      my $ids = {};
> >      my $order = {};
> > -
> > -    $raw = '' if !defined($raw);
> > -
> >      my $digest = Digest::SHA::sha1_hex($raw);
> >  
> > -    my $pri = 1;
> > +    my $current_order = 1;
>
> this is actually a worse name than before. this would be something like
> current_index? current_position? section_no?

Hm, agreed - those are actually better. Will incorporate in v2, thanks!

>
> > +    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;
>
> what do we gain from this?

My intention here was to work on the array directly and then return it
as a reference below (if there are any errors) rather than working with
a reference from the get-go, having to deref whenever there's a `push`.

I guess it's more a personal preference of mine, I just find it better
to work with. Can keep the array ref in v2 though, doesn't matter that
much. :P

>
> >  
> >      my $is_array = sub {
> >  	my ($type, $key) = @_;
> > @@ -1043,106 +1044,112 @@ 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;
> > +	}
> > +
> > +	# 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);
> >  
> > -	    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;
> > +		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_order++;
> >  	}
> >      }
> >  
> >      my $cfg = {
> >  	ids => $ids,
> >  	order => $order,
> > -	digest => $digest
> > +	digest => $digest,
> >      };
> > -    $cfg->{errors} = $errors if scalar($errors->@*) > 0;
> > +
> > +    $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 mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



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

[pve-devel] applied: [PATCH v1 pve-common 0/3] Section Config: Documentation & Code Cleanup

[Fiona Ebner]

Am 04.06.24 um 11:28 schrieb Max Carrara:
> 
> Max Carrara (3):
>   section config: document package and its methods with POD
>   section config: update code style
>   section config: clean up parser logic
> 
>  src/PVE/SectionConfig.pm | 982 +++++++++++++++++++++++++++++++--------
>  1 file changed, 798 insertions(+), 184 deletions(-)
> 

For the record, it seems like this already got applied:
https://git.proxmox.com/?p=pve-common.git;a=commit;h=6cba8d7660b62002ffdc81655b8e8668952614a9

Could you re-send the changes for v2 as follow-ups? Or v1 could also be
reverted and v2 applied?


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

Re: [pve-devel] applied: [PATCH v1 pve-common 0/3] Section Config: Documentation & Code Cleanup

[Fabian Grünbichler]

On July 24, 2024 1:34 pm, Fiona Ebner wrote:
> Am 04.06.24 um 11:28 schrieb Max Carrara:
>> 
>> Max Carrara (3):
>>   section config: document package and its methods with POD
>>   section config: update code style
>>   section config: clean up parser logic
>> 
>>  src/PVE/SectionConfig.pm | 982 +++++++++++++++++++++++++++++++--------
>>  1 file changed, 798 insertions(+), 184 deletions(-)
>> 
> 
> For the record, it seems like this already got applied:
> https://git.proxmox.com/?p=pve-common.git;a=commit;h=6cba8d7660b62002ffdc81655b8e8668952614a9
> 
> Could you re-send the changes for v2 as follow-ups? Or v1 could also be
> reverted and v2 applied?

I reverted them for now - it seems they accidentally slipped in when I
applied an unrelated patch cause of a dirty repo state I missed before
pushing..


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