[pve-devel] [PATCH 2/2] now if the QMP command starts with guest-+ , it will bind dynamicly to the VMID.qga socket. To test the function vmtime is implemented which return the vm UNIX Time.

Dietmar Maurer dietmar at proxmox.com
Mon Nov 24 16:59:53 CET 2014


First, thanks for the patch!

Further comments inline:

> Signed-off-by: Wolfgang Link <wolfgang at linksystems.org>
> ---
>  PVE/QMPClient.pm  |   64 +++++++++++++++++++++++++++++++-------------------
> ---
>  PVE/QemuServer.pm |   32 +++++++++++++++------------
>  qm                |   31 ++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+), 41 deletions(-)
> 
> diff --git a/PVE/QMPClient.pm b/PVE/QMPClient.pm index 9674d00..ab32a39
> 100755
> --- a/PVE/QMPClient.pm
> +++ b/PVE/QMPClient.pm
> @@ -20,7 +20,7 @@ use Data::Dumper;
>  # Note: kvm can onyl handle 1 connection, so we close connections asap
> 
>  sub new {
> -    my ($class, $eventcb, $qga) = @_;
> +    my ($class, $eventcb) = @_;
> 
>      my $mux = new IO::Multiplex;
> 
> @@ -34,7 +34,6 @@ sub new {
>      }, $class;
> 
>      $self->{eventcb} = $eventcb if $eventcb;
> -    $self->{qga} = $qga if $qga;
> 
>      $mux->set_callback_object($self);
> 
> @@ -124,10 +123,12 @@ my $close_connection = sub {  };
> 
>  my $open_connection = sub {
> -    my ($self, $vmid, $timeout) = @_;
> -
> -    my $sname = PVE::QemuServer::qmp_socket($vmid, $self->{qga});
> +    my ($self, $vmid, $timeout, $qga) = @_;
> 
> +    my $sname = PVE::QemuServer::qmp_socket($vmid);
> +
> +    $sname = PVE::QemuServer::qga_socket($vmid) if $qga;

I would simply use:

my $sname = PVE::QemuServer::qmp_socket($vmid, $qga);

> +
>      $timeout = 1 if !$timeout;
> 
>      my $fh;
> @@ -191,19 +192,21 @@ my $check_queue = sub {
> 
>  	    my $qmpcmd = undef;
> 
> -	    if($self->{qga}){
> +	    if($self->{current}->{$vmid}->{qga}){
> +
> +		my $qmpcmdid = undef;
> 
> -		my $qmpcmdid =to_json({
> +		$qmpcmdid =to_json({

This change is not really required?

>  		    execute => 'guest-sync',
> -		    arguments => { id => int($cmd->{id})}});
> -
> +		    arguments => { id => int ($cmd->{id}) }});

what is the difference?

> +
>  		$qmpcmd = to_json({
>  		    execute => $cmd->{execute},
>  		    arguments => $cmd->{arguments}});
> -
> +

another nop?

>  		$qmpcmd = $qmpcmdid.$qmpcmd;
> 
> -	    }else{
> +	    } else{

If you want to correct coding style, this should be

+	    } else {

But in general, it is better to do that with a separate patch.

> 
>  		$qmpcmd = to_json({
>  		    execute => $cmd->{execute},
> @@ -243,12 +246,17 @@ sub queue_execute {
>      foreach my $vmid (keys %{$self->{queue}}) {
>  	next if !scalar(@{$self->{queue}->{$vmid}}); # no commands for the VM
> 
> +	if ($self->{queue}->{$vmid}[0]->{execute} =~ /^guest\-+/){
> +	    $self->{queue}->{$vmid}[0]->{qga} = "1";
> +	}
> +
>  	eval {
> -	    my $fh = &$open_connection($self, $vmid, $timeout);
> -
> -	     if(!$self->{qga}){
> -		 my $cmd = { execute => 'qmp_capabilities', arguments => {} };
> -		 unshift @{$self->{queue}->{$vmid}}, $cmd;
> +	    my $fh = &$open_connection($self, $vmid, $timeout,
> +$self->{queue}->{$vmid}[0]->{qga} );
> +
> +	    if (!$self->{queue}->{$vmid}[0]->{qga}){
> +		my $cmd = { execute => 'qmp_capabilities', arguments => {} };
> +		unshift @{$self->{queue}->{$vmid}}, $cmd;
> +	    }
> 
>  	    $self->{mux}->set_timeout($fh, $timeout);
>  	};
> @@ -256,8 +264,8 @@ sub queue_execute {
>  	    warn $err;
>  	    $self->{errors}->{$vmid} = $err;
>  	}
> -	}

white-space change

>      }
> +

white-space change

>      my $running;
> 
>      for (;;) {
> @@ -290,16 +298,18 @@ sub mux_close {
>  # the descriptors.
>  sub mux_input {
>      my ($self, $mux, $fh, $input) = @_;
> -
> -    if($self->{qga}){
> -	return if $$input !~ m/}\n(.+)}\n$/;
> -    }else{
> -	return if $$input !~ m/}\r\n$/;
> -    }
> +
> +    my $vmid = $self->{fhs_lookup}->{$fh};
> 
> -    my $raw = $$input;
> +    my $raw;
> +    if ($self->{current}->{$vmid}->{qga}) {
> +	return if $$input !~ s/^(.+}\n.+})\n(.*)$/$2/so;

Maybe we can be more restrictive here:

return if $$input !~ s/^([^\n]+}\n[^\n]+})\n(.*)$/$2/so;

> +	$raw = $1;
> +    } else {
> +	return if $$input !~ s/^(.*})\r?\n(.*)$/$2/so;

same here:

return if $$input !~ s/^([^\n]+})\r?\n(.*)$/$2/so;

> +	$raw = $1;
> +    }
> 
> -    my $vmid = $self->{fhs_lookup}->{$fh};
>      if (!$vmid) {
>  	warn "internal error - unable to lookup vmid";
>  	return;
> @@ -308,7 +318,7 @@ sub mux_input {
>      eval {
>  	my @jsons = split("\n", $raw);
> 
> -	if($self->{qga}){
> +	if($self->{current}->{$vmid}->{qga}){
> 
>  	    die "response is not complete" if @jsons != 2 ;
> 
> @@ -321,7 +331,7 @@ sub mux_input {
> 
>  	    delete $self->{current}->{$vmid};
> 
> -	    if ($curcmd->{id} ne $cmdid) {
> +	    if (int ($curcmd->{id}) ne $cmdid) {

why is this required?

>  		die "got wrong command id '$cmdid' (expected $curcmd-
> >{id})\n";
>  	    }
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index d740564..4709382
> 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2871,9 +2871,8 @@ sub spice_port {
>  }
> 
>  sub qmp_socket {
> -    my ($vmid, $qga) = @_;
> -    my $sockettype = $qga ? 'qga' : 'qmp';
> -    return "${var_run_tmpdir}/$vmid.$sockettype";
> +    my ($vmid) = @_;
> +    return "${var_run_tmpdir}/$vmid.qmp";
>  }

There is no need to change that.

> 
>  sub qga_socket {
> @@ -2881,13 +2880,6 @@ sub qga_socket {
>      return "${var_run_tmpdir}/$vmid.qga";  }
> 
> -sub vm_qga_cmd {
> -    my ($vmid, $execute, %params) = @_;
> -
> -    my $cmd = { execute => $execute, arguments => \%params };
> -    vm_qmp_command($vmid, $cmd, undef, 1);
> -}
> -
>  sub pidfile_name {
>      my ($vmid) = @_;
>      return "${var_run_tmpdir}/$vmid.pid"; @@ -3466,6 +3458,18 @@ sub
> vm_start {
>      });
>  }
> 
> +sub get_vmtime {
> +
> +    my ($vmid) = @_;
> +
> +    my $qmpclient = PVE::QMPClient->new();
> +
> +    my $cmd = {'execute' => 'guest-get-time'} ;
> +
> +    return $qmpclient->cmd($vmid, $cmd); }
> +
> +

Please use a separate patch to implement 'vmtime' API.

>  sub vm_mon_cmd {
>      my ($vmid, $execute, %params) = @_;
> 
> @@ -3481,7 +3485,7 @@ sub vm_mon_cmd_nocheck {  }
> 
>  sub vm_qmp_command {
> -    my ($vmid, $cmd, $nocheck, $qga) = @_;
> +    my ($vmid, $cmd, $nocheck) = @_;
> 
>      my $res;
> 
> @@ -3493,12 +3497,12 @@ sub vm_qmp_command {
> 
>      eval {
>  	die "VM $vmid not running\n" if !check_running($vmid, $nocheck);
> -	my $sname = qmp_socket($vmid, $qga);
> +	my $sname = qmp_socket($vmid);
>  	if (-e $sname) {
> -	    my $qmpclient = PVE::QMPClient->new(undef, $qga);
> +	    my $qmpclient = PVE::QMPClient->new();
> 
>  	    $res = $qmpclient->cmd($vmid, $cmd, $timeout);
> -	} elsif (-e "${var_run_tmpdir}/$vmid.mon" && !$qga) {
> +	} elsif (-e "${var_run_tmpdir}/$vmid.mon") {
>  	    die "can't execute complex command on old monitor - stop/start your
> vm to fix the problem\n"
>  		if scalar(%{$cmd->{arguments}});
>  	    vm_monitor_command($vmid, $cmd->{execute}, $nocheck); diff --git
> a/qm b/qm index cea223e..1af9550 100755
> --- a/qm
> +++ b/qm
> @@ -44,6 +44,11 @@ my $upid_exit = sub {
> 
>  my $nodename = PVE::INotify::nodename();
> 
> +my $dump_output = sub {
> +    my $output = shift;
> +    print $output."\n";
> +};
> +
>  sub run_vnc_proxy {
>      my ($path) = @_;
> 
> @@ -387,6 +392,30 @@ __PACKAGE__->register_method ({
>  	return undef;
>      }});
> 
> +__PACKAGE__->register_method ({
> +    name => 'vmtime',
> +    path => 'vmtime',
> +    method => 'GET',
> +    description => "Get the systemtime from the VMID vm')",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    vmid => get_standard_option('pve-vmid'),
> +	},
> +    },
> +    returns => { type => 'string'},
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = $param->{vmid};
> +
> +	my $conf = PVE::QemuServer::load_config ($vmid); # check if VM exists
> +
> +	die "VM $vmid is not running\n" if
> +!PVE::QemuServer::check_running($vmid);
> +
> +	return  PVE::QemuServer::get_vmtime($vmid);
> +    }});
> +
>  my $cmddef = {
>      list => [ "PVE::API2::Qemu", 'vmlist', [],
>  	     { node => $nodename }, sub {
> @@ -476,6 +505,8 @@ my $cmddef = {
>      mtunnel => [ __PACKAGE__, 'mtunnel', []],
> 
>      terminal => [ __PACKAGE__, 'terminal', ['vmid']],
> +
> +    vmtime => [ __PACKAGE__, 'vmtime', ['vmid'], {}, $dump_output],
>  };
> 
>  my $cmd = shift;
> --
> 1.7.10.4
> 





More information about the pve-devel mailing list