[pve-devel] [PATCH storage 2/2] refactor finding next diskname for all plugins

[Wolfgang Bumiller]

On Fri, Aug 24, 2018 at 05:01:32PM +0200, Thomas Lamprecht wrote:
> On 8/22/18 3:42 PM, Stoiko Ivanov wrote:
> > +    my $dh = IO::Dir->new ($imgdir);
> > +    @$disk_list = $dh->read() if defined($dh);

I'd prefer an enclosing if() scope here. Won't make much of a difference
at this point, but it's nice to limit the scope of variables referencing
resources:
    if (defined($dh = IO::Dir->new($imgdir))) {
        @$disk_list = $dh->read();
    }

(...)
> > -    die "unable to allocate an image name for ID $vmid in storage '$storeid'\n";
> > +    my $disk_list = [ keys(%{$lvs->{$vg}}) ];
> 
> maybe remove the parenthesis from keys to make it look less like a
> special chars ABC soup :)

At this point I have to throw in the big question: With stretch the
postfix dereference syntax variant doesn't throw warnings anymore, so
do we ever want to - for statements already using an arrow to
dereference something - allow (or even prefer) this style?

    my $disk_list = [ keys $lvs->{$vg}->%* ];

I generally prefer operations being listed in order rather than going
left-right-left-right as if we were writing cdecls ;-)

IMO it's still fine for the above construct as there's only one set of
braces for dereferencing a hash. It only becomes unreadable when doing
things like ${%{${a[b]}}{c}}{d} anyway, which we don't.