[pve-devel] [PATCH access-control] RFC: add ls command for pvum user

[Thomas Lamprecht]

Am 05/07/2018 um 02:00 PM schrieb Stoiko Ivanov:
 > partially addresses #1502
 >
 > Given that this is my first patch to this list I would appreciate 
feedback!
 >

on top of Dietmars feedback, some comments inline, mostly coding style
nits, the code semantics itself are looking OK.

 > ---
 >   PVE/CLI/pveum.pm | 19 +++++++++++++++++++
 >   1 file changed, 19 insertions(+)
 >
 > diff --git a/PVE/CLI/pveum.pm b/PVE/CLI/pveum.pm
 > index a4e584d..f744398 100755
 > --- a/PVE/CLI/pveum.pm
 > +++ b/PVE/CLI/pveum.pm
 > @@ -42,6 +42,25 @@ our $cmddef = {
 >   	add    => [ 'PVE::API2::User', 'create_user', ['userid'] ],
 >   	modify => [ 'PVE::API2::User', 'update_user', ['userid'] ],
 >   	delete => [ 'PVE::API2::User', 'delete_user', ['userid'] ],
 > +	ls => [ 'PVE::API2::User', 'index', [], {}, sub {
 > +	    my $userlist = shift;
 > +	    printf "%-30s %-7s\n",
I'd omit this line break, looks a harder to read as is, IMO.

 > +	    qw(USERID ENABLED);
 > +	    foreach my $user (sort{ $a->{userid} cmp $b->{userid}} 
@$userlist) {
whitespace between sort and the opening { bracket.


 > +		printf "%-30s %-7d\n", $user->{userid}, $user->{enable}//1;
space before and after the //, makes it easier to read.

 > +	    }
 > +	    } ],
 > +	get => [ 'PVE::API2::User', 'read_user', ['userid'], undef, sub {
maybe show or dump instead of get? not sure though...

 > +	    my $userdata = shift;
 > +	    foreach my $item (sort keys %$userdata) {
 > +		if( ref($userdata->{$item}) eq 'ARRAY'){
we always put a space after if/elsif and the opening ( parenthesis:

if (check) {
    foo()
} elsif (check_two) {
    bar()
} else {
    die "...";
}

 > +		    printf "%s: [ %s ]\n", $item, join ", ",
this line break could probably omitted, often in our code base we allow
going over the 80 characters line length limit if it just a little bit
and avoids, less readable, line breaks. There's no clear rule and both
is OK, but in this case it'd be better if no line break is there,
obviously IMO as this is somewhat subjective. You could also just name
the variable $user here, it's expressive enough and shorter.

Oh and please use join() - note the parenthesis, makes it clearer here.

 > +		    @{$userdata->{$item}};
 > +		} else {
 > +		    printf "%s: %-7s\n", $item, $userdata->{$item};
 > +		}
 > +	    }
 > +	    } ],
 >       },
 >       group => {
 >   	add    => [ 'PVE::API2::Group', 'create_group', ['groupid'] ],
 >