[pve-devel] [pve-manager] pvesr set_state

[Thomas Lamprecht]

On 06/09/2017 08:19 AM, Wolfgang Link wrote:
> ---
>   PVE/CLI/pvesr.pm | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>
> diff --git a/PVE/CLI/pvesr.pm b/PVE/CLI/pvesr.pm
> index f64a3103..ca8cd59c 100644
> --- a/PVE/CLI/pvesr.pm
> +++ b/PVE/CLI/pvesr.pm
> @@ -288,6 +288,38 @@ __PACKAGE__->register_method ({
>   	return PVE::API2::ReplicationConfig->update($param);
>       }});
>   
> +__PACKAGE__->register_method ({
> +    name => 'set_state',
> +    path => '',
> +    protected => 1,
> +    method => 'POST',
> +    description => "Set the job replication state on migration. This call is for internal use. It will accept the job state as ja JSON obj.",
> +    permissions => {
> +	check => ['perm', '/storage', ['Datastore.Allocate']],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::Cluster::complete_vmid }),
> +	    state => {
> +		description => "Job state as JSON decoded string.",
> +		type => 'string',
> +	    },
> +	},
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = extract_param($param, 'vmid');
> +	my $json_string = extract_param($param, 'state');
> +	my $remote_job_state= decode_json $json_string;

That looks all in all quite unsafe?! state can be an arbitrary string,
if it's not valid json 'decode_json' catches it, but the error messages 
isn't quite nice, e.g.:
# malformed JSON string, neither tag, array, object, number, string or 
atom, at character offset 0 (before "foo") at -e line 1.


Even if valid JSON it isn't ensured that it's a valid replication state 
JSON object
(you do not input check $state in the set_remote_statemethod from the 
other patch)
Isn't there a schematic definition about how a state could look like, 
i.e. which entries are valid?

Further it's isn't checked if vmid exists or if there is even a job for 
that VMID where such a state could be altered, is this OK?
set_remote_state doesn't check for this either...

> +
> +	PVE::ReplicationState::set_remote_state($remote_job_state, $vmid);
> +	return undef;
> +    }});
> +
> +
>   my $print_job_list = sub {
>       my ($list) = @_;
>   
> @@ -359,6 +391,7 @@ our $cmddef = {
>       'finalize-local-job' => [ __PACKAGE__, 'finalize_local_job', ['id', 'extra-args'], {} ],
>   
>       run => [ __PACKAGE__ , 'run'],
> +    'set-state' => [ __PACKAGE__ , 'set_state', ['vmid', 'state']],
>   };
>   
>   1;