On 4/7/20 5:55 PM, Thomas Lamprecht wrote:
> On 4/6/20 4:24 PM, Aaron Lauterer wrote:
>> [...]
>> + description => 'Root node of the tree object. Children
represent guests, grandchildren represent volumes of that guest.',
>> + properties => {
>> + not_all_permissions => {
>> + type => 'boolean',
>> + optional => 1,
>> + description => 'Wheter the user is missing permissions to
view all guests.',
>
> s/Wheter/Whether/
thanks for catching that typo
>
>> + },
>> + children => {
>> + type => 'array',
>> + items => {
>> + type => 'object',
>> + properties => {
>> + id => {
>> + type => 'integer',
>> + description => 'VMID of the guest.',
>> + },
>> + name => {
>> + type => 'string',
>> + description => 'Name of the guest',
>> + optional => 1,
>> + },
>> + type => {
>> + type => 'string',
>> + description => 'Type of the guest, VM or CT.',
>> + enum => ['qemu', 'lxc', 'unknown'],
>
> We die in the unknown case, so that cannot be returned
right
>
>> + },
>> + children => {
>> + type => 'array',
>> + optional => 1,
>> + description => 'The volumes of the guest with the
information if they will be included in backups.',
>> + items => {
>> + type => 'object',
>> + properties => {
>> + id => {
>> + type => 'string',
>> + description => 'Configuration key of the volume.',
>> + },
>> + name => {
>> + type => 'string',
>> + description => 'Name of the volume.',
>> + },
>> + included => {
>> + type => 'boolean',
>> + description => 'Wheter the volume is included
in the backup or not.',
>> + },
>> + reason => {
>> + type => 'string',
>> + description => 'The reason why the volume is
included (or excluded).',
>> + },
>> + },
>> + },
>> + },
>> + },
>> + },
>> + },
>> + },
>> + },
>> + code => sub {
>> + my ($param) = @_;
>> +
>> + my $rpcenv = PVE::RPCEnvironment::get();
>> +
>> + my $user = $rpcenv->get_user();
>> +
>> + my $vzconf = cfs_read_file('vzdump.cron');
>> + my $all_jobs = $vzconf->{jobs} || [];
>> + my $job;
>> + my $rrd = PVE::Cluster::rrd_dump();
>> +
>> + foreach my $j (@$all_jobs) {
>> + $job = $j if $j->{id} eq $param->{id};
>
> This could let the reader think that this is a bug, as it looks like
it gets
> overwritten, plus on huge amount of jobs you would iterate a lot of
those for
> nothing, if the $job with the requested id was found early. Rather do:
>
> if ($j->{id} eq $param->{id}) {
> $job = $j;
> last;
> }
>
> This makes it much more explicit and easier to grasp when just
quickly reading over
> the code.
will do
>
>> + }
>> + raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
>> +
>> + my $vmlist = PVE::Cluster::get_vmlist();
>> +
>> + my @vmids = @{PVE::VZDump->get_included_guests($job)};
>
> s/vmid/job_vmids/
>
> and the comment on the other series regarding passing this as reference,
> and even as hash.
Yeah, this will be adapted to the changes of the other series, one of
the reasons why this is labeled as RFC
>
>> +
>> + # remove VMIDs to which the user has no permission to not leak
infos
>> + # like the guest name
>> + my $not_all_permissions = 0;
>> + @vmids = grep {
>> + my $allowed = $rpcenv->check($user, "/vms/$_", [
'VM.Backup' ], 1);
>
> hmm, is VM.Backup really the info we want to hide, why not VM.Audit?
> (or those which the user has either permission?)
>
> As VM.Backup is the permission for being allowed to make a backup or
restore one,
> not for being allowed to view a VM in the cluster.
I haven't fully groked the permission system yet. What I want to achieve
is to not show any VMs the user does not have permission to see.
I think VM.Audit should be enough then.
>
>> + $not_all_permissions = 1 if !$allowed && !$job->{all};
>
> This could be found out afterwards, avoids a O(n) expression.
$job->{all} is
> available anyway and if you remember how many elements @vmids had
before you
> can just check if it has less after the permission checking. That has the
> additional advantage that you can omit the $allowed intermediate
variable. For
> example (not fully thought out - so don't just copy 1:1)
>
> my $job_guest_count = scalar(@vmids);
> my @allowed_vmids = grep {
> $rpcenv->check_any($user, "/vms/$_", [ 'VM.Audit', 'VM.Backup' ], 1)
> } @vmids;
>
> my $permissions_for_all = $job->{all} || $job_guest_count ==
scalar(@allowed_vmids);
thx for the hint. I'll look into it.
>
>> + $allowed;
>> + } @vmids;
>> +
>> + my $result = {
>> + children => [],
>> + leaf => 0,
>
> can't that be determined indirectly? I.e., if children is empty? (or
maybe made explicitly undef/null)
I checked the ExtJS docs again and IIUC the leaf flag only needs to be
set if it is true. Thus we can remove it here altogether. But just out
of curiosity; would it be a bad pattern to set it here to the most
likely value (guests usually do have volumes) and check at the end after
all possible volumes are added if there are any children, and if not,
set leaf to 1?
>
>> + not_all_permissions => $not_all_permissions,
>> + };
>> +
>> + foreach (@vmids) {
>> + my $id = $_;
>
> This is a style we normally do not use, do:
>
> for my $vmid (@vmids) {
okay
>
>> +
>> + # it's possible that a job has VMIDs configured that are not in
>> + # vmlist. This could be because a guest was removed but not
purged.
>> + # Since there is no more data available we can only deliver
the VMID
>> + # and no volumes.
>> + if (!defined $vmlist->{ids}->{$id}) {
>> + my $missing_guest = {
>> + id => $id,
>> + type => 'unknown',
>> + leaf => 1,
>> + };
>> + push @{$result->{children}}, $missing_guest;
>> + next;
>> + }
>> +
>> + my $guest = {
>> + id => $id + 0,
>
> here you do + 0 (which i rather have as int($x)) but a few lines
above you don't?
> I'd guess that it isn't required here?
It's possible that I forgot it above but I will check again if it is
stored as a number or string and use int() if needed.
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
