Hi,
the attached test cases show some small problems with uninitialized values.
Please use them only in a test environment. It might destroy pools, VMs or
backup jobs.
On Thu, Dec 12, 2019 at 11:27:44AM +0100, Aaron Lauterer wrote:
> + my $vzconf = cfs_read_file('vzdump.cron');
> + my $jobs = $vzconf->{jobs} || [];
> + my $job;
> +
> + foreach my $j (@$jobs) {
> + $job = $j if $j->{id} eq $param->{id};
> + }
Maybe something like s/$jobs/$cronjobs/ or required_id=$param->{id}? If we
could make
the difference between j, job, jobs, $vzconf->{jobs} a little clearer that
would be nice.
> + raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
> +
> + my @vmids;
> + my $vmlist = PVE::Cluster::get_vmlist();
> +
> + # get VMIDs from pool or selected individual guests
> + if ($job->{pool}) {
> + @vmids =
> @{PVE::API2Tools::get_resource_pool_guest_members($job->{pool})};
> + } elsif ($job->{vmid}) {
> + @vmids = PVE::Tools::split_list(extract_param($job, 'vmid'));
> + }
When reading this I would have to know the whole context to know why there is
no else.
I personally would rather write
if ($job->{pool}) {
...
} else {
# now $job->{vmid} is defined
}
or as you did further below
if () {
} elsif () {
} else {
# must not happen, throw some error
}
> +
> + # get excluded guests
> + my @exclude = PVE::Tools::split_list(extract_param($job, 'exclude'));
> + @exclude = @{PVE::VZDump::check_vmids(@exclude)};
> + @vmids = @{PVE::VZDump::check_vmids(@vmids)};
> +
> + my $excludehash = { map { $_ => 1 } @exclude };
> +
> + # no list of guests? (from pool or manually)
> + # get all except excluded
> + if (!@vmids) {
> + if ($job->{all} && $job->{node}) {
> + foreach my $id (keys %{ $vmlist->{ids} }) {
> + push @vmids, $id
> + if $vmlist->{ids}->{$id}->{node} eq $job->{node} &&
> !$excludehash->{$id};
> + }
> + } elsif ($job->{all}) {
> + foreach my $id (keys %{ $vmlist->{ids} }) {
> + push @vmids, $id if !$excludehash->{$id};
> + }
Same question as elsif above
> + }
> + }
> +
> + @vmids = sort {$a <=> $b} @vmids;
> +
> + # prepare API response
This is already clear from the variable name for me.
> + my $result = {
> + children => [],
> + leaf => 0,
> + };
> +
> + foreach (@vmids) {
> + my $id = $_;
> + my $type = $vmlist->{ids}->{$id}->{type};
> + my $node = $vmlist->{ids}->{$id}->{node};
> +
> + my $guest = {
> + id => $id,
> + children => [],
> + leaf => 0,
> + };
> +
> + if ($type eq 'qemu') {
> + my $conf = PVE::QemuConfig->load_config($id, $node);
> +
> + $guest->{id} = $guest->{id} . " " . $conf->{name};
> + $guest->{type} = 'VM';
> +
> + PVE::QemuServer::foreach_drive($conf, sub {
> + my ($key, $attr) = @_;
> +
> + return if PVE::QemuServer::drive_is_cdrom($attr);
> +
> + my $status = 'true';
> +
> + $status = 'false' if (exists($attr->{backup}) &&
> !$attr->{backup});
> +
> + my $disk = {
> + id => $key . ' - '. $attr->{file},
> + status => $status,
> + leaf => 1,
> + };
> + push @{$guest->{children}}, $disk;
> + });
> + } elsif ($type eq 'lxc') {
> + my $conf = PVE::LXC::Config->load_config($id, $node);
> +
> + $guest->{id} = $guest->{id} . " " . $conf->{hostname};
> + $guest->{type} = 'CT';
> +
> + PVE::LXC::Config->foreach_mountpoint($conf, sub {
> + my ($key, $attr) = @_;
> +
> + my $status = 'false';
> +
> + $status = 'true' if ($key eq 'rootfs' ||
> (exists($attr->{backup}) && $attr->{backup}));
> + $status = 'not possible' if ($attr->{type} ne 'volume');
> +
> + my $disk = {
> + id => $key . ' - '. $attr->{volume},
> + status => $status,
> + leaf => 1,
> + };
> + push @{$guest->{children}}, $disk;
> + });
> + } else {
> + die "VMID $id is not Qemu nor LXC guest\n";
Neither ... nor ... ?
Nontheless, this is whole section is very clearly written IMO :)
> + }
> +
> + push @{$result->{children}}, $guest;
> + }
> +
> + return $result;
> +}});
> 1;
> --
> 2.20.1
>
>
> ___
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
