[pve-devel] [PATCH access-control 1/3] Add parameter userlist to pveum for listing users

[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 | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/PVE/CLI/pveum.pm b/PVE/CLI/pveum.pm
> index aef7089..0426cfb 100755
> --- a/PVE/CLI/pveum.pm
> +++ b/PVE/CLI/pveum.pm
> @@ -18,6 +18,7 @@ use PVE::API2::ACL;
>   use PVE::API2::AccessControl;
>   use PVE::JSONSchema qw(get_standard_option);
>   use PVE::CLIHandler;
> +use POSIX qw(strftime);
>   
>   use base qw(PVE::CLIHandler);
>   
> @@ -49,6 +50,26 @@ our $cmddef = {
>       useradd => [ 'PVE::API2::User', 'create_user', ['userid'] ],
>       usermod => [ 'PVE::API2::User', 'update_user', ['userid'] ],
>       userdel => [ 'PVE::API2::User', 'delete_user', ['userid'] ],
> +    userlist => [ 'PVE::API2::User', 'index', undef, undef, sub {
> +	    my $userlist = shift;
> +
> +	    exit 0 if (!scalar(@$userlist));
Exiting in sub methods is not considered good practice and should only be
used if its really desired, just returning is better here.

The method may be called in any context, where you may not know what after
it happens. E.g. you could circumvent cleanup procedures as you exit the
whole program not just this function context.
This specific case would not be too bad, but its not needed and could cause
implication if moved (e.g. to API), copied or if our CLIHandler starts to
need executing functions after this.

> +
> +	    my $format = qq(%15s %-5s %-8s %-10s %-15s %-30s %-30s\n);
> +	    printf($format, qw(USERID REALM ENABLED EXPIRE NAME EMAIL COMMENT));

We had/have different heading styles, the consensus was to mainly use "normal"
capitalization where applicable, e.g.:

UserID Realm Enabled Expiration Name Email Comment

pvesm and pct use above naming scheme, qm uses the 'shouting' one.
Personally I can live with both, but it should be consistent.

> +> +	    foreach my $rec (sort {$a->{userid} cmp $b->{userid} } @$userlist) {
> +		my $expire = $rec->{expire} eq 0 ? "" : (strftime("%Y-%m-%d",localtime($rec->{expire})));

use < > == != <= >= for integer comparison, not eq
maybe use something like:

	my $expire = 'never';
	$expire = strftime("%Y-%m-%d",localtime($rec->{expire})) if $res->{expire};

its a scheme we use on a few places, first set the default then the value, if given.
Here the '> 0' is not directly necessary as with this if we guarantee that expire is
defined and not 0, can be still added if wanted and/or to highlight what we really
check for.

> +		my $fullname = join(" ", ($rec->{firstname} || ""), ($rec->{lastname} || ""));> +		$fullname =~ s/^\s+//;


I'd either just have separate columns for Name and Surname making above obsolete
or:

	my $fullname = $rec->{firstname} || '';
	$fullname .= " $rec->{lastname}" if $rec->{lastname};

> +		my ($userid, $realm) = split('@', $rec->{userid});
> +
> +		printf($format, $userid, $realm, $rec->{enable},
> +		$expire, $fullname, ($rec->{email} || ""),
> +		($rec->{comment} || ""));

You often use the ($res->{foo} || '') scheme, maybe using an little inline helper could
make this nicer?

	my $get = sub { return $rec->{$_[0]} || '' };

$rec comes from the outside context and you pass the wanted key as parameter, $_[0]
is just away to access the first param without adding intermediate variables, in
small helper we may use it to keep them really small and its clear what it does.

Then you could do:

	$get->('email'), $extract->('comment'), ...

> +	    }
> +
> +	} ],
>   
>       groupadd => [ 'PVE::API2::Group', 'create_group', ['groupid'] ],
>       groupmod => [ 'PVE::API2::Group', 'update_group', ['groupid'] ],
>