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

[Thomas Lamprecht]

On 06/09/2017 09:59 AM, Thomas Lamprecht wrote:
>
>
> 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...
>


Dietmar told my that this is an internal API only exposed over CLI, so 
my comments can be somewhat disregarded.
Although I'd still do some input/error checks, imo this is always good 
and helps to prevent bugs or unwanted API misuse.


>> +
>> +    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;
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel