On 1/16/20 4:40 PM, Stefan Reiter wrote:
> Turn CPUConfig into a SectionConfig with parsing/writing support for
> custom CPU models. IO is handled using cfs.
>
> Namespacing will be provided using "custom-" prefix for custom model
> names (in VM config only, cpu-models.conf will contain unprefixed
> names).
>
> Includes two overrides to avoid writing redundant information to the
> config file, additionally get_custom_model is used to retrieve a custom
> model configuration by name.
>
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>
> Depends on updated pve-cluster.
>
looks OK, a few nits inline - do not warrant a v8 but if other things
warrant one you could address them too.
>
> PVE/QemuServer/CPUConfig.pm | 109 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 106 insertions(+), 3 deletions(-)
>
> diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
> index 86febe8..36a9439 100644
> --- a/PVE/QemuServer/CPUConfig.pm
> +++ b/PVE/QemuServer/CPUConfig.pm
> @@ -4,15 +4,26 @@ use strict;
> use warnings;
>
> use PVE::JSONSchema;
> +use PVE::Cluster qw(cfs_register_file cfs_read_file);
> use PVE::QemuServer::Helpers qw(min_version);
>
> -use base qw(Exporter);
> +use base qw(PVE::SectionConfig Exporter);
>
> our @EXPORT_OK = qw(
> print_cpu_device
> get_cpu_options
> );
>
> +mkdir "/etc/pve/virtual-guest";
> +my $default_filename = "virtual-guest/cpu-models.conf";
> +cfs_register_file($default_filename,
> + sub { PVE::QemuServer::CPUConfig->parse_config(@_); },
> + sub { PVE::QemuServer::CPUConfig->write_config(@_); });
> +
> +sub load_custom_model_conf {
> + return cfs_read_file($default_filename);
> +}
> +
> my $cpu_vendor_list = {
> # Intel CPUs
> 486 => 'GenuineIntel',
> @@ -84,11 +95,20 @@ my $cpu_flag = qr/[+-](@{[join('|', @supported_cpu_flags)]})/;
>
> our $cpu_fmt = {
> cputype => {
> - description => "Emulated CPU type.",
> + description => "Emulated CPU type. Can be default or custom name (custom model names must be prefixed with 'custom-').",
> type => 'string',
> - enum => [ sort { "\L$a" cmp "\L$b" } keys %$cpu_vendor_list ],
> + format_description => 'string',
> default => 'kvm64',
> default_key => 1,
> + optional => 1,
> + },
> + 'reported-model' => {
> + description => "CPU model and vendor to report to the guest. Must be a QEMU/KVM supported model."
> + . " Only valid for custom CPU model definitions, default models will always report themselves to the guest OS.",
> + type => 'string',
> + enum => [ sort { lc("$a") cmp lc("$b") } keys %$cpu_vendor_list ],
> + default => 'kvm64',
> + optional => 1,
> },
> hidden => {
> description => "Do not identify as a KVM virtual machine.",
> @@ -114,6 +134,86 @@ our $cpu_fmt = {
> },
> };
>
> +# Section config settings
> +my $defaultData = {
> + # shallow copy, since SectionConfig modifies propertyList internally
> + propertyList => { %$cpu_fmt },
> +};
> +
> +sub private {
> + return $defaultData;
> +}
> +
> +sub options {
> + return { %$cpu_fmt };
> +}
> +
> +sub type {
> + return 'cpu-model';
> +}
> +
> +sub parse_section_header {
> + my ($class, $line) = @_;
> +
> + my ($type, $sectionId, $errmsg, $config) =
> + $class->SUPER::parse_section_header($line);
> +
> + return undef if !$type;
> + return ($type, $sectionId, $errmsg, {
> + # to avoid duplicate model name in config file, parse id as cputype
> + # models parsed from file are by definition always custom
comment is a bit hard to understand, maybe just add
"# cfg models are custom by definition" to the same line below?
> + cputype => "custom-$sectionId",
> + });
> +}
> +
> +sub write_config {
> + my ($class, $filename, $cfg) = @_;
> +
> + for my $model (keys %{$cfg->{ids}}) {
> + my $model_conf = $cfg->{ids}->{$model};
> +
> + die "internal error: tried saving built-in CPU model (or missing prefix)\n"
I always try to include the "bad" values which triggered this exception, so we've
get have something to work with if a user gets this message in a, for they our us,
unexpected situation
> + if !is_custom_model($model_conf->{cputype});
> +
> + die "internal error: tried saving custom cpumodel with cputype (ignoring prefix) not equal to \$cfg->ids entry\n"
same here, even if it gets a bit long then.
> + if "custom-$model" ne $model_conf->{cputype};
> +
> + # saved in section header
> + delete $model_conf->{cputype};
> + }
> +
> + $class->SUPER::write_config($filename, $cfg);
> +}
> +
> +sub is_custom_model {
> + my ($cputype) = @_;
> + return $cputype =~ m/^custom-/;
> +}
> +
> +# Use this to get a single model in the format described by $cpu_fmt.
> +# Allows names with and without custom- prefix.
> +sub get_custom_model {
> + my ($name, $noerr) = @_;
> +
> + $name =~ s/^custom-//;
> + my $conf = load_custom_model_conf();
> +
> + my $entry = $conf->{ids}->{$name};
> + if (!defined($entry)) {
> + die "Custom cputype '$name' not found\n" if !$noerr;
> + return undef;
> + }
> +
> + my $model = {};
> + for my $property (keys %$cpu_fmt) {
> + if (my $value = $entry->{$property}) {
> + $model->{$property} = $value;
> + }
> + }
> +
> + return $model;
> +}
> +
> # Print a QEMU device node for a given VM configuration for hotplugging CPUs
> sub print_cpu_device {
> my ($conf, $id) = @_;
> @@ -229,4 +329,7 @@ sub add_hyperv_enlightenments {
> }
> }
>
> +__PACKAGE__->register();
> +__PACKAGE__->init();
> +
> 1;
>