Nit: the qemu-server patches should come first, since the UI patches depend on them. And it's always good to mention in the cover letter which inter-package dependencies there are, i.e. pve-manager needs a versioned dependency bump for qemu-server. It's quite obvious in this case, but still helps and mostly writing this for the future.
The prefix 'qemu' doesn't really add any information here. It should rather be something like 'cpu config:' Am 12.03.26 um 9:39 AM schrieb Arthur Bied-Charreton: > Add functionality to lock & write the custom CPU config and some other > helpers that will be needed in custom CPU models CRUD endpoints. > > Original patch: > https://lore.proxmox.com/pve-devel/[email protected]/ > > Originally-by: Stefan Reiter <[email protected]> If you want, you can put a short high-level changelog here to document how the patch changed, e.g.: [AB: split patch into adding helpers and adding API endpoints some other change] This can help follow history better later on. > Signed-off-by: Arthur Bied-Charreton <[email protected]> > --- > src/PVE/QemuServer/CPUConfig.pm | 35 ++++++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > > diff --git a/src/PVE/QemuServer/CPUConfig.pm b/src/PVE/QemuServer/CPUConfig.pm > index 32ec4954..2b6665a7 100644 > --- a/src/PVE/QemuServer/CPUConfig.pm > +++ b/src/PVE/QemuServer/CPUConfig.pm > @@ -6,10 +6,10 @@ use warnings; > use JSON; > > use PVE::JSONSchema qw(json_bool); > -use PVE::Cluster qw(cfs_register_file cfs_read_file); > +use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file > cfs_lock_file); > use PVE::ProcFSTools; > use PVE::RESTEnvironment qw(log_warn); > -use PVE::Tools qw(run_command); > +use PVE::Tools qw(run_command lock_file); > > use PVE::QemuServer::Helpers qw(min_version); > > @@ -51,6 +51,19 @@ sub load_custom_model_conf { > return cfs_read_file($default_filename); > } > > +sub write_custom_model_conf { > + my ($conf) = @_; > + cfs_write_file($default_filename, $conf); Pre-existing, but could you add a preparatory commit renaming the variable $default_filename to something more telling? Maybe $cpu_models_filename? > +} > + > +sub lock_cpu_config { > + my ($code) = @_; > + cfs_lock_file($default_filename, undef, $code); > + if (my $err = $@) { > + die $err; > + } Style nit: could also just be die $@ if $@; > +} > + > #builtin models : reported-model is mandatory > my $builtin_models_by_arch = { > x86_64 => { > @@ -298,6 +311,19 @@ sub get_supported_cpu_flags { > return $supported_cpu_flags_by_arch->{$arch}; > } > > +sub description_by_flag { > + my ($arch) = @_; > + return { map { $_->{name} => $_->{description} } > get_supported_cpu_flags($arch)->@* }; > +} > + > +sub get_cpu_vendor { > + my ($cputype, $arch) = @_; > + $arch = $host_arch if !defined($arch); The $host_arch variable was recently dropped, needs a rebase. > + my $retval = $cpu_models_by_arch->{$arch}->{$cputype} > + or die "Built-in CPU type '$cputype' does not exist"; > + return $retval; > +} These are quite small helpers and I do not see a big reason why they should be added in a separate commit rather than just directly in the commits with their users. It can be okay to add helpers up-front, depending on the scenario, but here: * 2 of the helpers are for config file handling * 2 of the helpers are for cpu model property details, but also quite unrelated * the helpers are not complex So I don't see enough semantic grouping that would justify having this as a separate patch. > + > my $all_supported_cpu_flags = {}; > for my $arch ($supported_cpu_flags_by_arch->%*) { > for my $flag ($supported_cpu_flags_by_arch->{$arch}->@*) { > @@ -375,6 +401,7 @@ my $cpu_fmt = { > optional => 1, > }, > }; > +PVE::JSONSchema::register_standard_option('pve-qemu-cpu', $cpu_fmt); I'd rather have this as a separate commit or squashed into the commit with its first user. > > my $sev_fmt = { > type => { > @@ -564,6 +591,7 @@ sub write_config { > > # saved in section header > delete $model_conf->{cputype}; > + $model_conf->{type} = $class->type(); The commit message should at least describe why this is necessary. I suppose for writing to actually work? Could also be a separate fix then, but its fine to add together with the helper. > } > > $class->SUPER::write_config($filename, $cfg); > @@ -612,7 +640,8 @@ sub get_cpu_models { > > my $conf = load_custom_model_conf(); > for my $custom_model (keys %{ $conf->{ids} }) { > - my $reported_model = > $conf->{ids}->{$custom_model}->{'reported-model'}; > + my $model = $conf->{ids}->{$custom_model}; > + my $reported_model = $model->{'reported-model'}; This change is unrelated to the rest. > $reported_model //= $cpu_fmt->{'reported-model'}->{default}; > my $vendor = $all_cpu_models->{$reported_model}; > push @$models,
