[pve-devel] [PATCH] convert monitor_command to qmp_command - part1

Dietmar Maurer dietmar at proxmox.com
Wed Jun 6 12:16:20 CEST 2012


There are too many question in this patch, and still white space errors:

Applying: convert monitor_command to qmp_command - part1
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:15: space before tab in indent.
  	    my $ejectcmd = {};
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:30: trailing whitespace, space before tab in indent.
  	    	    my $changecmd = {};	
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:84: space before tab in indent.
    	        $self->log('info', "migration status: $ms (transferred ${trans}KB, " .
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:163: trailing whitespace.
	
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:252: trailing whitespace.
	
warning: 5 lines add whitespace errors.

So I will not commit that.

Also, this patch is quite large. If possible, try to assemble smaller ones.

Comments are inline:

> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index
> c172fde..d10db8c 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -673,12 +673,24 @@ my $vmconfig_update_disk = sub {
>      if (PVE::QemuServer::drive_is_cdrom($drive)) { # cdrom
> 
>  	if (PVE::QemuServer::check_running($vmid)) {
> +  	    my $ejectcmd = {};
> +	    $ejectcmd->{execute} = "eject";
> +	    $ejectcmd->{arguments}->{force} = bless( do{\(my $o = 1)},
> 'JSON::XS::Boolean' );

Can't we us '1', or JSON::true ?

> +	    $ejectcmd->{arguments}->{device} = "drive-$opt";
> +
>  	    if ($drive->{file} eq 'none') {
> -		PVE::QemuServer::vm_monitor_command($vmid, "eject -f
> drive-$opt", 0);
> +		PVE::QemuServer::vm_qmp_command($vmid, $ejectcmd, 0);
>  	    } else {
> +		PVE::QemuServer::vm_qmp_command($vmid, $ejectcmd, 0);
> #force eject if
> +locked
>  		my $path = PVE::QemuServer::get_iso_path($storecfg, $vmid,
> $drive->{file});
> -		PVE::QemuServer::vm_monitor_command($vmid, "eject -f
> drive-$opt", 0); #force eject if locked
> -		PVE::QemuServer::vm_monitor_command($vmid, "change
> drive-$opt \"$path\"", 0) if $path;
> +
> +		if($path) {
> +  	    	    my $changecmd = {};
> +		    $changecmd->{execute} = "change";
> +		    $changecmd->{arguments}->{device} = "drive-$opt";
> +		    $changecmd->{arguments}->{target} = $path;
> +		    PVE::QemuServer::vm_qmp_command($vmid,
> $changecmd, 0);
> +		}
>  	    }
>  	}
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm index
> 0dc81a5..2a3d3a5 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -324,29 +324,28 @@ sub phase2 {
>      # start migration
> 
>      my $start = time();
> +    my $migratecommand->{execute} = "migrate";
> +    $migratecommand->{arguments}->{uri} = "tcp:localhost:$lport";
> +    my $merr = PVE::QemuServer::vm_qmp_command($vmid,
> $migratecommand,
> + 1);
> 
> -    my $merr = PVE::QemuServer::vm_monitor_command($vmid, "migrate -d
> \"tcp:localhost:$lport\"", 1);
> -
> -    my $lstat = '';
>      while (1) {
>  	sleep (2);
> -	my $stat = PVE::QemuServer::vm_monitor_command($vmid, "info
> migrate", 1);
> -	if ($stat =~ m/^Migration status:
> (active|completed|failed|cancelled)$/im) {
> +	my $stat = PVE::QemuServer::vm_qmp_command($vmid, "query-
> migrate", 1);
> +
> +	if ($stat->{status} =~ m/^(active|completed|failed|cancelled)$/im) {
>  	    $merr = undef;
> -	    my $ms = $1;
> -
> -	    if ($stat ne $lstat) {	

We only want to print changes. Can we get back that functionality somehow?

> -		if ($ms eq 'active') {
> -		    my ($trans, $rem, $total) = (0, 0, 0);
> -		    $trans = $1 if $stat =~ m/^transferred ram: (\d+)
> kbytes$/im;
> -		    $rem = $1 if $stat =~ m/^remaining ram: (\d+) kbytes$/im;
> -		    $total = $1 if $stat =~ m/^total ram: (\d+) kbytes$/im;
> -
> -		    $self->log('info', "migration status: $ms (transferred
> ${trans}KB, " .
> -			       "remaining ${rem}KB), total ${total}KB)");
> -		} else {
> -		    $self->log('info', "migration status: $ms");
> -		}
> +	    my $ms = $stat->{status};
> +
> +	    if ($ms eq 'active') {
> +	        my ($trans, $rem, $total) = (0, 0, 0);
> +	        $trans = sprintf "%.2f", $stat->{ram}->{transferred}/1024 if $stat-
> >{ram}->{transferred};
> +	        $rem = sprintf "%.2f", $stat->{ram}->{remaining}/1024 if $stat-
> >{ram}->{remaining};
> +	        $total = sprintf "%.2f", $stat->{ram}->{total}/1024 if
> +$stat->{ram}->{total};
> +
> +    	        $self->log('info', "migration status: $ms (transferred ${trans}KB, "
> .
> +		           "remaining ${rem}KB), total ${total}KB)");
> +	    } else {
> +	        $self->log('info', "migration status: $ms");
>  	    }
> 
>  	    if ($ms eq 'completed') {
> @@ -363,10 +362,9 @@ sub phase2 {
> 
>  	    last if $ms ne 'active';
>  	} else {
> -	    die $merr if $merr;
> -	    die "unable to parse migration status '$stat' - aborting\n";
> +	    die $merr->{desc} if $merr->{desc};
> +	    die "unable to parse migration status '$stat->{status}' -
> +aborting\n";
>  	}
> -	$lstat = $stat;
>      };
>  }
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index
> 47c122e..c608312 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2593,7 +2593,17 @@ sub qemu_block_set_io_throttle {
>      $iops_rd = 0 if !$iops_rd;
>      $iops_wr = 0 if !$iops_wr;
> 
> -    my $ret = vm_monitor_command($vmid, "block_set_io_throttle $deviceid
> $bps $bps_rd $bps_wr $iops $iops_rd $iops_wr");
> +    my $cmd = {};
> +    $cmd->{execute} = "block_set_io_throttle";
> +    $cmd->{arguments}->{device} = $deviceid;
> +    $cmd->{arguments}->{bps} = $bps;
> +    $cmd->{arguments}->{bps_rd} = $bps_rd;
> +    $cmd->{arguments}->{bps_wr} = $bps_wr;
> +    $cmd->{arguments}->{iops} = $iops;
> +    $cmd->{arguments}->{iops_rd} = $iops_rd;
> +    $cmd->{arguments}->{iops_wr} = $iops_wr;	


That code look quite ugly for me. Maybe we can add wrappers for vm_qmp_command():

for example:

vm_mon_cmd($vmid, $execute, %params)
vm_mon_cmd_nocheck($vmid, $execute, %params)

so we can call it like:

vm_mon_cmd($vmid, 'block_set_io_throttle', device => $deviceid, bps => $bps, iops => $iops, ...)

What do you think?

> +
> +    my $ret = vm_qmp_command($vmid, $cmd);
>      $ret =~ s/^\s+//;
>      return 1 if $ret eq "";
>      syslog("err", "error setting block_set_io_throttle: $ret"); @@ -2653,7
> +2663,7 @@ sub vm_start {
>  	    } else {
>  		unlink $statefile;
>  		# fixme: send resume - is that necessary ?
> -		eval { vm_monitor_command($vmid, "cont"); };
> +		eval { vm_qmp_command($vmid, "cont"); };
>  	    }
>  	}
> 
> @@ -2662,31 +2672,38 @@ sub vm_start {
>  	my $migrate_speed = $defaults->{migrate_speed} || 8192;
>  	$migrate_speed = $conf->{migrate_speed} || $migrate_speed;
>  	eval {
> -	    my $cmd = "migrate_set_speed ${migrate_speed}m";
> -	    vm_monitor_command($vmid, $cmd);
> +	    my $cmd = {};
> +	    $cmd->{execute} = "migrate_set_speed";
> +	    $cmd->{arguments}->{value} = ($migrate_speed * 1048576);
> #value
> +in bytes with qmp
> +
> +	    vm_qmp_command($vmid, $cmd);
>  	};
> 
>  	my $migrate_downtime = $defaults->{migrate_downtime};
>  	$migrate_downtime = $conf->{migrate_downtime} if defined($conf-
> >{migrate_downtime});
>  	if (defined($migrate_downtime)) {
> -	    my $cmd = "migrate_set_downtime ${migrate_downtime}";
> -	    eval { vm_monitor_command($vmid, $cmd); };
> +	    my $cmd = {};
> +	    $cmd->{execute} = "migrate_set_downtime";
> +	    $cmd->{arguments}->{value} = $migrate_downtime;
> +
> +	    eval { vm_qmp_command($vmid, $cmd); };
>  	}
> 
>  	vm_balloonset($vmid, $conf->{balloon}) if $conf->{balloon};
> -
> +
> +	vm_qmp_command($vmid,"query-commands");
>      });
>  }
> 
>  my $qmp_read_avail = sub {
> -    my ($fh, $timeout) = @_;
> +    my ($fh, $timeout, $returnerror) = @_;
> 
>      my $sel = new IO::Select;
>      $sel->add($fh);
> 
>      my $res = '';
>      my $buf;
> -
> +    $timeout = 3600;

Why do you overwrite timeout here? value is passed as parameter!

>      my @ready;
>      while (scalar (@ready = $sel->can_read($timeout))) {
>  	my $count;
> @@ -2702,8 +2719,18 @@ my $qmp_read_avail = sub {
>      }
> 
>      die "qmp read timeout\n" if !scalar(@ready);
> -    my $obj = from_json($res);
> -    return $obj;
> +    my @jsons = split("\n", $res);
> +    my $obj = {};
> +    my $event = '';
> +    foreach my $json (@jsons) {
> +	my $obj = from_json($json);
> +	$event = $obj->{event} if exists $obj->{event};
> +
> +	return $obj->{error} if exists $obj->{error} && $returnerror;
> +	die $obj->{error}->{desc} if exists $obj->{error}->{desc};
> +	return $obj->{QMP} if exists $obj->{QMP};
> +	return $obj->{"return"} if exists $obj->{"return"};
> +    }

Wow. I simply do not understand that code. For example $event is set but never used?
I guess this is worth a separate patch including some docs (inline comments).

>  };
> 
>  sub __read_avail {
> @@ -2831,6 +2858,9 @@ sub vm_qmp_command {
> 
>  	# hack: migrate sometime blocks the monitor (when
> migrate_downtime
>  	# is set)
> +	if (ref($cmdstr) eq "HASH") {
> +	   $timeout = 60*60 if ($cmdstr->{execute} =~ m/(migrate)$/);
> +	}
>  	#if ($cmdstr =~ m/^(info\s+migrate|migrate\s)/) {
>  	#    $timeout = 60*60; # 1 hour
>  	#}
> @@ -2838,7 +2868,7 @@ sub vm_qmp_command {
>  	# read banner;
>  	my $data = &$qmp_read_avail($sock, $timeout);
>  	# '{"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 1},
> "package": " (qemu-kvm-devel)"}, "capabilities": []}} ';
> -	die "got unexpected qemu qmp banner\n" if !$data->{QMP};
> +	die "got unexpected qemu qmp banner\n" if !$data;
> 
>  	my $sel = new IO::Select;
>  	$sel->add($sock);
> @@ -2857,18 +2887,19 @@ sub vm_qmp_command {
> 
>          $res = &$qmp_read_avail($sock, $timeout);
>          #  res = '{"return": {}}
> -        die "qmp negociation error\n" if !$res->{return};
> +        die "qmp negociation error\n" if !$res;
> 
>  	$timeout = 20;
> 
>  	my $fullcmd;
> +	my $returnerror;
>  	if (ref($cmdstr) eq "HASH") {
>  	    #generate json from hash for complex cmd
>  	    $fullcmd = to_json($cmdstr);
> -
> -	    if ($fullcmd->{execute}  =~ m/^(info\s+migrate|migrate\s)/) {
> +	    if ($cmdstr->{execute}  =~ m/(migrate)$/) {
>  		$timeout = 60*60; # 1 hour
> -	    } elsif ($fullcmd->{execute} =~ m/^(eject|change)/) {
> +		$returnerror = 1;	

Would you mind to explain that?

For me it looks easier to catch the error in  QemuMigrate.pm, and remove that
whole returnerror code:

    eval { PVE::QemuServer::vm_qmp_command($vmid, $migratecommand, 1); }
    my $merr = $@;

or do we need that functionality somewhere else?

> +	    } elsif ($cmdstr->{execute} =~ m/^(eject|change)/) {
>  		$timeout = 60; # note: cdrom mount command is slow
>  	    }
>  	} else {
> @@ -2883,8 +2914,8 @@ sub vm_qmp_command {
>  	if (ref($cmdstr) ne "HASH") {
>  	    return if ($cmdstr eq 'q') || ($cmdstr eq 'quit');
>  	}
> -
> -	$res = &$qmp_read_avail($sock, $timeout);
> +
> +	$res = &$qmp_read_avail($sock, $timeout, $returnerror);
>      };
> 

- Dietmar




More information about the pve-devel mailing list