hi, thanks for the review!
On Thu, Jan 23, 2020 at 07:41:11AM +0100, Thomas Lamprecht wrote: > On 1/22/20 5:37 PM, Oguz Bektas wrote: > > this patch adds the partial_fast_plug function, which allows to partially > > fastplug an option with a property string. > > > > this is done by having a map $partial_fast_plug_option, the format is > > commented. > > > > other helper functions: > > > > * safe_boolean_ne (!$a != !$b) > > * typesafe_ne (combines safe_string_ne, safe_num_ne and safe_boolean_ne by > > taking $type as an argument) > > > > the qemu-guest-agent is our first use case for this (although i am sure > > there are more, this is more of a proof of concept. it should be trivial > > to add other things via the map), specifically the fstrim_cloned_disks > > option. > > > > Co-Authored-by: Stefan Reiter <s.rei...@proxmox.com> > > Signed-off-by: Oguz Bektas <o.bek...@proxmox.com> > > --- > > > > i added stefan as a co-author for this, to thank for his help debugging and > > testing it with me > > no thanks for me providing the initial hunk of code doing (almost) all > of this ? ;P > > > > > also please note that i'm sending this as RFC for review only, and it > > shouldn't be applied yet since i'm working on generalizing it a bit more > > for code reuse via pve-guest-common > > As you only call that method in the agent case this is also not ready to > be taken in here. Some other issues below. > > > Testing changing the agent from a deactivated to a "change every setting" > leads > to an error with your series: > > agent: hotplug problem - format error enabled: property is missing and it > > is not optional > > That happens because $old is empty in that case but you try to parse it as > format > nonetheless. will fix that, thanks > > > > > PVE/QemuServer.pm | 78 +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 78 insertions(+) > > > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > > index bcdadca..74cbba0 100644 > > --- a/PVE/QemuServer.pm > > +++ b/PVE/QemuServer.pm > > @@ -5031,6 +5031,10 @@ sub vmconfig_hotplug_pending { > > } > > vmconfig_update_disk($storecfg, $conf, > > $hotplug_features->{disk}, > > $vmid, $opt, $value, 1, $arch, > > $machine_type); > > + } elsif ($opt eq 'agent') { > > + # partially fastpluggable > > useless comment okay > > > + # skip if all options were fastpluggable > > No! You do the contrary, you skip if *not* all options were fastpluggable! right. i'll change the comment > > You could *always* skip if you deleted $cond->{pending}->{$opt} in > partial_fast_plug > if $no_changes there is false. If you want to do it like this and let the > deletion > from pending happen here, OK, but get the comments right - else this is > confusing > as hell. i'd like to let the hotplug_pending path handle the deletion from pending. i'll fix the comments in the new version also based on our offline discussion this belongs in the else branch at the end, i'll also take a look at that > > > + die "skip\n" if partial_fast_plug($conf, $opt); > > } elsif ($opt =~ m/^memory$/) { #dimms > > die "skip\n" if !$hotplug_features->{memory}; > > $value = PVE::QemuServer::Memory::qemu_memory_hotplug($vmid, > > $conf, $defaults, $opt, $value); > > @@ -5165,6 +5169,80 @@ my $safe_string_ne = sub { > > return $a ne $b; > > }; > > > > +my $safe_boolean_ne = sub { > > + my ($a, $b) = @_; > > + > > + # we don't check if value is defined, since undefined > > + # is false (so it's a valid boolean) > > The comment below is enough, above adds just noise - it's a common enough > pattern, IMO. okay > > > + # negate both values to normalize and compare > > + return !$a != !$b; > > +}; > > + > > +my $typesafe_ne = sub { > > + my ($a, $b, $type) = @_; > > + > > + return 0 if !defined($a) && !defined($b); > > + return 1 if !defined($a); > > + return 1 if !defined($b); > > + > > + if ($type eq 'string') { > > + $safe_string_ne->($a, $b); > > 1. The returns are still missing, as told off-list. missing those works by > chance only > and is very prone for subtle breakage. right, i forgot about those, i'll change that as well > > 2. if you move this to guest-common do the check directly here, no point in > calling (and > then also moving that) method for a single line. I.e., just do here: i thought i'd move all of it to guest-common to have both guest types reuse (to avoid possible future inconsistencies between implementations). right now we have them duplicated in both > > return $a ne $b; > > > + } elsif ($type eq 'number') { > > + $safe_num_ne->($a, $b); > > return $a != $b; > > > + } elsif ($type eq 'boolean') { > > + $safe_boolean_ne->($a, $b); > > return !$a != !$b; > > directly. > > > + } > > +}; > > + > > +my $partial_fast_plug_option = > > +# name > > +# -> fmt -> format variable > > +# -> properties -> fastpluggable options hash > > move above variable def. > > > +{ > > this belongs at the line of variable def okay > > > + agent => { > > + fmt => $agent_fmt, > > Maybe we could/should really get the format from the $confdesc and simplify > this > to > my $partial_fast_plug_option = { > agent => { > fstrim_cloned_disks => 1 > }, > }; yeah that also makes sense, as long as there's no $format outside of $confdesc. i'll check that and change it accordingly > > + properties => { > > + fstrim_cloned_disks => 1 > > + }, > > + }, > > +}; > > + > > +sub partial_fast_plug { > > + my ($conf, $opt) = @_; > > + > > + my $format = $partial_fast_plug_option->{$opt}->{fmt}; > > + my $properties = $partial_fast_plug_option->{$opt}->{properties}; > > if you would call this $fast_pluggable instead of this over general name > you could avoid that extra $is_fastpluggable variable below. okay > > > + > > + my $old = PVE::JSONSchema::parse_property_string($format, > > $conf->{$opt}); > > my $old = {}; > if (exists($conf->{$opt})) { > $old = PVE::JSONSchema::parse_property_string($format, $conf->{$opt}); > } > > Else you throw up if $conf->{$opt} was not set previously. indeed. > > > + my $new = PVE::JSONSchema::parse_property_string($format, > > $conf->{pending}->{$opt}); > > above should be OK as the pending $opt has to be always set, else this mustn't > be called. > > > + > > + my $changes_left = 0; > > + > > + # merge old and new opts to iterate > > + my $all_opts = dclone($old); > > + foreach my $opt1 (keys %$new) { > > + $all_opts->{$opt1} = $new->{$opt1} if !defined($all_opts->{$opt1}); > > + } > > Instead of above 5 lines you could just do: > > my @all_keys = keys %{{ %$new, %$old }}; > > (you only need the keys, not the data) nice, i didn't know about that syntax > > > + > > + foreach my $opt2 (keys %$all_opts) { > > you can reuse variable names from for(each) in different scopes, no need > to use $opt1, $opt2, ... having $opt in all the loops made it look a bit confusing since $opt is also in the function parameters, that's why i didn't reuse it. seemed easier to read this way if you insist i can change it though > > > + my $is_fastpluggable = $properties->{$opt2}; > > + my $type = $format->{$opt2}->{type}; > > + if ($typesafe_ne->($old->{$opt2}, $new->{$opt2}, $type)) { > > + if (defined($is_fastpluggable)) { > > why defined check? We control that definition, so normal "boolean" check is > enough, > with above suggested changes together just write: > > if ($fast_pluggable->{$opt}) { > ... > will take a look at that > > + $old->{$opt2} = $new->{$opt2}; > > + } else { > > + $changes_left = 1; > > + } > > + } > > + } > > + > > + $conf->{$opt} = PVE::JSONSchema::print_property_string($old, $format); > > + > > + return $changes_left; > > +} > > + > > + > > sub vmconfig_update_net { > > my ($storecfg, $conf, $hotplug, $vmid, $opt, $value, $arch, > > $machine_type) = @_; > > > > > _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel