Thanks for the review! On 2023-03-15 15:05, Wolfgang Bumiller wrote: > On Wed, Mar 01, 2023 at 10:12:26AM +0100, Leo Nunner wrote: >> Implements support for passing values to the fw_cfg argument for QEMU. >> If the value looks like a file, the backend checks for the correct >> permissions/path and if everything is right, includes it as a file >> instead of as a string. >> >> Setting the argument requires the VM.Config.Options permission on the >> guest. Including files requires Datastore.Audit and Datastore.Allocate >> on the specific storage. >> >> Signed-off-by: Leo Nunner <[email protected]> >> --- >> RFC: I feel like a more implicit option for passing files would be >> nicer, but I can't really think of a nice way… >> >> PVE/API2/Qemu.pm | 14 ++++++++++++++ >> PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++ >> 2 files changed, 49 insertions(+) >> >> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm >> index 587bb22..c03394f 100644 >> --- a/PVE/API2/Qemu.pm >> +++ b/PVE/API2/Qemu.pm >> @@ -639,6 +639,8 @@ my $check_vm_modify_config_perm = sub { >> # the user needs Disk and PowerMgmt privileges to change the vmstate >> # also needs privileges on the storage, that will be checked later >> $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk', >> 'VM.PowerMgmt' ]); >> + } elsif ($opt eq 'fw_cfg') { >> + $rpcenv->check_vm_perm($authuser, $vmid, $pool, >> ['VM.Config.Options']); >> } else { >> # catches hostpci\d+, args, lock, etc. >> # new options will be checked here >> @@ -1770,6 +1772,18 @@ my $update_vm_api = sub { >> } elsif ($opt eq 'tags') { >> assert_tag_permissions($vmid, $conf->{$opt}, >> $param->{$opt}, $rpcenv, $authuser); >> $conf->{pending}->{$opt} = >> PVE::GuestHelpers::get_unique_tags($param->{$opt}); >> + } elsif ($opt eq 'fw_cfg') { >> + foreach my $fw_cfg (PVE::Tools::split_list($param->{$opt})) >> { >> + my ($opt, $val) = split("=", $fw_cfg); >> + >> + if (my $storage = PVE::Storage::parse_volume_id($val, >> 1)) { >> + $rpcenv->check($authuser, "/storage/$storage", >> ['Datastore.Audit', 'Datastore.Allocate']); >> + >> + my ($path, undef, $type) = >> PVE::Storage::path($storecfg, $val); >> + die "File $val is not in snippets directory\n" if >> $type ne "snippets"; >> + } >> + } >> + $conf->{pending}->{$opt} = $param->{$opt}; >> } else { >> $conf->{pending}->{$opt} = $param->{$opt}; >> >> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm >> index 40be44d..c5dea1f 100644 >> --- a/PVE/QemuServer.pm >> +++ b/PVE/QemuServer.pm >> @@ -723,6 +723,12 @@ EODESCR >> description => "List of host cores used to execute guest processes, for >> example: 0,5,8-11", >> optional => 1, >> }, >> + fw_cfg => { >> + type => 'string', >> + optional => 1, >> + description => 'Pass values to the guest via the fw_cfg parameter.', >> + format => 'pve-fw-cfg-list', >> + }, >> }; >> >> my $cicustom_fmt = { >> @@ -1076,6 +1082,21 @@ sub verify_volume_id_or_absolute_path { >> return $volid; >> } >> >> +PVE::JSONSchema::register_format('pve-fw-cfg', \&verify_fw_cfg); >> +sub verify_fw_cfg { >> + my ($value, $noerr) = @_; >> + >> + my $FW_CFG_REGEX = qr/[a-z0-9\.\/:\-]+/; > IMO this is way too restrictive. If we pass the `name=` prefix in the > `-fw_cfg` parameter we can even allow `=`. In fact, AFAICT only `,` is > problematic and would need to be doubled. Also if we really just have a > single option with a list... both `,` and `;` will be problematic on our > end, so it's fine to exclude those 2 for now. (Until we add quoting > support to split_list ;-) ) > > That said, for the use cases I have in mind for us, the current code > would be sufficient, but at least uppercase letters, underscores and > spaces would be nice... We could also just switch to something like `[^,;]+`, or would that be too extreme? >> + >> + if ($value =~ m/^(opt\/$FW_CFG_REGEX)=($FW_CFG_REGEX)$/) { >> + return $value; >> + } >> + >> + return if $noerr; >> + >> + die "unable to parse fw_cfg option\n"; >> +} >> + >> my $usb_fmt = { >> host => { >> default_key => 1, >> @@ -4181,6 +4202,20 @@ sub config_to_command { >> push @$cmd, '-snapshot'; >> } >> >> + if ($conf->{fw_cfg}) { >> + foreach my $conf (PVE::Tools::split_list($conf->{fw_cfg})) { >> + my ($opt, $val) = split("=", $conf); >> + >> + push @$cmd, "-fw_cfg"; >> + if (PVE::Storage::parse_volume_id($val, 1)) { >> + my $path = PVE::Storage::path($storecfg, $val); >> + push @$cmd, "$opt,file=$path"; >> + } else { >> + push @$cmd, "$opt,string=$val"; > ^ This would definitely be safer with the `name=` portion included. Agreed, I'll change it accordingly.
_______________________________________________ pve-devel mailing list [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
