[pve-devel] [PATCH qemu-server 2/5] use helper functions from GuestHelpers
Stefan Reiter
s.reiter at proxmox.com
Thu Feb 13 11:16:33 CET 2020
This patch needs a rebase to apply cleanly.
On 1/28/20 4:03 PM, Oguz Bektas wrote:
> removes safe_string_ne and safe_num_ne code which is now shared in
> GuestHelpers. also change all the calls
>
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
> PVE/QemuServer.pm | 82 ++++++++++++++++++-----------------------------
> 1 file changed, 31 insertions(+), 51 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 7374bf1..6a8dc16 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4969,26 +4969,6 @@ sub vmconfig_apply_pending {
> PVE::QemuConfig->write_config($vmid, $conf);
> }
>
> -my $safe_num_ne = sub {
> - my ($a, $b) = @_;
> -
> - return 0 if !defined($a) && !defined($b);
> - return 1 if !defined($a);
> - return 1 if !defined($b);
> -
> - return $a != $b;
> -};
> -
> -my $safe_string_ne = sub {
> - my ($a, $b) = @_;
> -
> - return 0 if !defined($a) && !defined($b);
> - return 1 if !defined($a);
> - return 1 if !defined($b);
> -
> - return $a ne $b;
> -};
> -
> sub vmconfig_update_net {
> my ($storecfg, $conf, $hotplug, $vmid, $opt, $value, $arch, $machine_type) = @_;
>
> @@ -4997,9 +4977,9 @@ sub vmconfig_update_net {
> if ($conf->{$opt}) {
> my $oldnet = parse_net($conf->{$opt});
>
> - if (&$safe_string_ne($oldnet->{model}, $newnet->{model}) ||
> - &$safe_string_ne($oldnet->{macaddr}, $newnet->{macaddr}) ||
> - &$safe_num_ne($oldnet->{queues}, $newnet->{queues}) ||
> + if (PVE::GuestHelpers::safe_string_ne($oldnet->{model}, $newnet->{model}) ||
> + PVE::GuestHelpers::safe_string_ne($oldnet->{macaddr}, $newnet->{macaddr}) ||
> + PVE::GuestHelpers::safe_num_ne($oldnet->{queues}, $newnet->{queues}) ||
> !($newnet->{bridge} && $oldnet->{bridge})) { # bridge/nat mode change
>
> # for non online change, we try to hot-unplug
> @@ -5010,19 +4990,19 @@ sub vmconfig_update_net {
> die "internal error" if $opt !~ m/net(\d+)/;
> my $iface = "tap${vmid}i$1";
>
> - if (&$safe_string_ne($oldnet->{bridge}, $newnet->{bridge}) ||
> - &$safe_num_ne($oldnet->{tag}, $newnet->{tag}) ||
> - &$safe_string_ne($oldnet->{trunks}, $newnet->{trunks}) ||
> - &$safe_num_ne($oldnet->{firewall}, $newnet->{firewall})) {
> + if (PVE::GuestHelpers::safe_string_ne($oldnet->{bridge}, $newnet->{bridge}) ||
> + PVE::GuestHelpers::safe_num_ne($oldnet->{tag}, $newnet->{tag}) ||
> + PVE::GuestHelpers::safe_string_ne($oldnet->{trunks}, $newnet->{trunks}) ||
> + PVE::GuestHelpers::safe_num_ne($oldnet->{firewall}, $newnet->{firewall})) {
> PVE::Network::tap_unplug($iface);
> PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
> - } elsif (&$safe_num_ne($oldnet->{rate}, $newnet->{rate})) {
> + } elsif (PVE::GuestHelpers::safe_num_ne($oldnet->{rate}, $newnet->{rate})) {
> # Rate can be applied on its own but any change above needs to
> # include the rate in tap_plug since OVS resets everything.
> PVE::Network::tap_rate_limit($iface, $newnet->{rate});
> }
>
> - if (&$safe_string_ne($oldnet->{link_down}, $newnet->{link_down})) {
> + if (PVE::GuestHelpers::safe_string_ne($oldnet->{link_down}, $newnet->{link_down})) {
> qemu_set_link_status($vmid, $opt, !$newnet->{link_down});
> }
>
> @@ -5066,32 +5046,32 @@ sub vmconfig_update_disk {
> # update existing disk
>
> # skip non hotpluggable value
> - if (&$safe_string_ne($drive->{discard}, $old_drive->{discard}) ||
> - &$safe_string_ne($drive->{iothread}, $old_drive->{iothread}) ||
> - &$safe_string_ne($drive->{queues}, $old_drive->{queues}) ||
> - &$safe_string_ne($drive->{cache}, $old_drive->{cache})) {
> + if (PVE::GuestHelpers::safe_string_ne($drive->{discard}, $old_drive->{discard}) ||
> + PVE::GuestHelpers::safe_string_ne($drive->{iothread}, $old_drive->{iothread}) ||
> + PVE::GuestHelpers::safe_string_ne($drive->{queues}, $old_drive->{queues}) ||
> + PVE::GuestHelpers::safe_string_ne($drive->{cache}, $old_drive->{cache})) {
> die "skip\n";
> }
>
> # apply throttle
> - if (&$safe_num_ne($drive->{mbps}, $old_drive->{mbps}) ||
> - &$safe_num_ne($drive->{mbps_rd}, $old_drive->{mbps_rd}) ||
> - &$safe_num_ne($drive->{mbps_wr}, $old_drive->{mbps_wr}) ||
> - &$safe_num_ne($drive->{iops}, $old_drive->{iops}) ||
> - &$safe_num_ne($drive->{iops_rd}, $old_drive->{iops_rd}) ||
> - &$safe_num_ne($drive->{iops_wr}, $old_drive->{iops_wr}) ||
> - &$safe_num_ne($drive->{mbps_max}, $old_drive->{mbps_max}) ||
> - &$safe_num_ne($drive->{mbps_rd_max}, $old_drive->{mbps_rd_max}) ||
> - &$safe_num_ne($drive->{mbps_wr_max}, $old_drive->{mbps_wr_max}) ||
> - &$safe_num_ne($drive->{iops_max}, $old_drive->{iops_max}) ||
> - &$safe_num_ne($drive->{iops_rd_max}, $old_drive->{iops_rd_max}) ||
> - &$safe_num_ne($drive->{iops_wr_max}, $old_drive->{iops_wr_max}) ||
> - &$safe_num_ne($drive->{bps_max_length}, $old_drive->{bps_max_length}) ||
> - &$safe_num_ne($drive->{bps_rd_max_length}, $old_drive->{bps_rd_max_length}) ||
> - &$safe_num_ne($drive->{bps_wr_max_length}, $old_drive->{bps_wr_max_length}) ||
> - &$safe_num_ne($drive->{iops_max_length}, $old_drive->{iops_max_length}) ||
> - &$safe_num_ne($drive->{iops_rd_max_length}, $old_drive->{iops_rd_max_length}) ||
> - &$safe_num_ne($drive->{iops_wr_max_length}, $old_drive->{iops_wr_max_length})) {
> + if (PVE::GuestHelpers::safe_num_ne($drive->{mbps}, $old_drive->{mbps}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{mbps_rd}, $old_drive->{mbps_rd}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{mbps_wr}, $old_drive->{mbps_wr}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops}, $old_drive->{iops}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops_rd}, $old_drive->{iops_rd}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops_wr}, $old_drive->{iops_wr}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{mbps_max}, $old_drive->{mbps_max}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{mbps_rd_max}, $old_drive->{mbps_rd_max}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{mbps_wr_max}, $old_drive->{mbps_wr_max}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops_max}, $old_drive->{iops_max}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops_rd_max}, $old_drive->{iops_rd_max}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops_wr_max}, $old_drive->{iops_wr_max}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{bps_max_length}, $old_drive->{bps_max_length}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{bps_rd_max_length}, $old_drive->{bps_rd_max_length}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{bps_wr_max_length}, $old_drive->{bps_wr_max_length}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops_max_length}, $old_drive->{iops_max_length}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops_rd_max_length}, $old_drive->{iops_rd_max_length}) ||
> + PVE::GuestHelpers::safe_num_ne($drive->{iops_wr_max_length}, $old_drive->{iops_wr_max_length})) {
I think putting the safe_X_ne/typesafe_ne in an EXPORT_OK would be worth
it, they have unique and unambiguous names and they're used quite a bit
in 2/5 and 3/5. This block of 18 calls in one 'if' is what gets me the
most, the PVE::GuestHelpers prefix isn't helping readability...
>
> qemu_block_set_io_throttle($vmid,"drive-$opt",
> ($drive->{mbps} || 0)*1024*1024,
>
More information about the pve-devel
mailing list