From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id E3C561FF13E for ; Fri, 20 Mar 2026 18:20:58 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 274371D267; Fri, 20 Mar 2026 18:21:15 +0100 (CET) Message-ID: Date: Fri, 20 Mar 2026 18:20:38 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Fiona Ebner Subject: Re: [PATCH qemu-server 6/8] qemu: Add helpers for new custom models endpoints To: Arthur Bied-Charreton , pve-devel@lists.proxmox.com References: <20260312084021.124465-1-a.bied-charreton@proxmox.com> <20260312084021.124465-7-a.bied-charreton@proxmox.com> Content-Language: en-US In-Reply-To: <20260312084021.124465-7-a.bied-charreton@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774027195361 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.060 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.408 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.819 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.903 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: QT44DR657H4BWWUBRZ3LGMR3MM32TQS4 X-Message-ID-Hash: QT44DR657H4BWWUBRZ3LGMR3MM32TQS4 X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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/20211028114150.3245864-5-s.reiter@proxmox.com/ > > Originally-by: Stefan Reiter 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 > --- > 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,