From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id BFAEB1FF136 for ; Mon, 23 Mar 2026 07:56:48 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F2D8DBC34; Mon, 23 Mar 2026 07:57:04 +0100 (CET) Date: Mon, 23 Mar 2026 07:56:56 +0100 From: Arthur Bied-Charreton To: Fiona Ebner Subject: Re: [PATCH qemu-server 6/8] qemu: Add helpers for new custom models endpoints Message-ID: <3ea5d2qsgvktyaqqbjtxrrrdvhngwzfqt7ftuslps7ab6dbdis@bpxvrnl4dfp2> References: <20260312084021.124465-1-a.bied-charreton@proxmox.com> <20260312084021.124465-7-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774248972264 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.861 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.001 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.001 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.001 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: I2KR2RU2CDCIFJADZKUIIB463J6IXAJ3 X-Message-ID-Hash: I2KR2RU2CDCIFJADZKUIIB463J6IXAJ3 X-MailFrom: a.bied-charreton@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 CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri, Mar 20, 2026 at 06:20:38PM +0100, Fiona Ebner wrote: > 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. > Makes sense, will reorder the patches in v2 > 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. > Noted, thanks! > > 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? > Will do > > +} > > + > > +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 $@; > Noted > > +} > > + > > #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. > Noted > > + 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. > Makes sense, will squash helpers into the commits that use them > > + > > 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. > Noted > > > > 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. > > > } Yes, this was in the original commit but I somehow lost it, sorry! Will be in v2. > > > > $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. > Will remove it, sorry about that. > > $reported_model //= $cpu_fmt->{'reported-model'}->{default}; > > my $vendor = $all_cpu_models->{$reported_model}; > > push @$models, >