[pve-devel] [PATCH v2 qemu-server 05/12] Add overrides and convenience functions to CPUConfig

[Thomas Lamprecht]

On 9/30/19 12:58 PM, Stefan Reiter wrote:
> Add two overrides to avoid writing redundant information to the config
> file.
> 
> get_model_by_name is used to return a cpu config with default values
> filled out.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>  PVE/QemuServer/CPUConfig.pm | 48 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
> index 3fde987..125b636 100644
> --- a/PVE/QemuServer/CPUConfig.pm
> +++ b/PVE/QemuServer/CPUConfig.pm
> @@ -157,6 +157,54 @@ 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
> +	cputype => $sectionId,
> +	# models parsed from file are by definition always custom
> +	custom => 1,
> +    });
> +}
> +
> +sub write_config {
> +    my ($class, $filename, $cfg) = @_;
> +
> +    # remove redundant properties

the comments from above parse_section_header are OK, but just looking at
this one is a bit confusing.. maybe also add here also something like

delete $cfg->{ids}->{$prop}->{custom}; # all models we write out are custom
delete $cfg->{ids}->{$prop}->{cputype}; # saved in the section header

also could it make sense to add asserts/checks that cputype == $prop and
custom == 1 ?

> +    for my $prop (keys %{$cfg->{ids}}) {
> +	delete $cfg->{ids}->{$prop}->{custom};
> +	delete $cfg->{ids}->{$prop}->{cputype};
> +    }
> +
> +    $class->SUPER::write_config($filename, $cfg);
> +}
> +
> +# Use this to get a single model in the format described by $cpu_fmt.

I'd add

# fills in defaults from schema if value is not set in config

> +# Returns undef for unknown $name.
> +sub get_model_by_name {
> +    my ($conf, $name) = @_;
> +
> +    return undef if !defined($conf->{ids}->{$name});
> +
> +    my $model = {};
> +    for my $property (keys %{$defaultData->{propertyList}}) {
> +	next if $property eq 'type';
> +
> +	if (my $value = $conf->{ids}->{$name}->{$property}) {

your hash access are a bit to long for my taste ^^

if it's used only once it can be OK, but else introduce some
intermediate variables, especially if over 3 hash layer accesses

at the start you could already do:
my $entry = $conf->{ids}->{$name};
return undef if !defined($entry);

and then use it here

> +	    $model->{$property} = $value;
> +	} elsif (my $default = $defaultData->{propertyList}->{$property}->{default}) {
> +	    $model->{$property} = $default;
> +	}
> +    }
> +
> +    return $model;
> +}
> +
>  # Print a QEMU device node for a given VM configuration for hotplugging CPUs
>  sub print_cpu_device {
>      my ($conf, $id) = @_;
>