[pve-devel] [PATCH access-control 2/3] Add grouplist to pveum for group listing

[Thomas Lamprecht]

comments inline

On 09/22/2017 04:32 PM, Alwin Antreich wrote:
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>   PVE/CLI/pveum.pm | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/PVE/CLI/pveum.pm b/PVE/CLI/pveum.pm
> index 0426cfb..7a4930f 100755
> --- a/PVE/CLI/pveum.pm
> +++ b/PVE/CLI/pveum.pm
> @@ -74,6 +74,18 @@ our $cmddef = {
>       groupadd => [ 'PVE::API2::Group', 'create_group', ['groupid'] ],
>       groupmod => [ 'PVE::API2::Group', 'update_group', ['groupid'] ],
>       groupdel => [ 'PVE::API2::Group', 'delete_group', ['groupid'] ],
> +    grouplist => [ 'PVE::API2::Group', 'index', undef, undef, sub {
> +	    my $grouplist = shift;
> +
> +	    exit 0 if (!scalar(@$grouplist));

same as in the first patch, use return

> +
> +	    my $format = qq(%15s  %-10s\n);

just use normal double quotes: "%15s  %-10s\n"

> +	    printf($format, qw(GROUP COMMENT));
> +
> +	    foreach my $rec (sort {$a->{groupid} cmp $b->{groupid} } @$grouplist) {
> +		printf($format, $rec->{groupid}, $rec->{comment});

Here you have no default handling for $rec->{comment} but in the first
patch you have such handling for the users comment entry.
Does this is OK if comment is undef here?

Oh, and as Wolfgang mentioned already in another answer to Philip, we would rather
see this moved above and just pass a code reference here.

> +	    }
> +	}],
>   
>       roleadd => [ 'PVE::API2::Role', 'create_role', ['roleid'] ],
>       rolemod => [ 'PVE::API2::Role', 'update_role', ['roleid'] ],
>