[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