On Wed, Feb 17, 2016 at 03:59:33PM +0100, Timo Grodzinski wrote:
> Hi Wolfgang,
>
> Thanks for your annotations!
>
> Am 17.02.2016 um 15:01 schrieb Wolfgang Bumiller:
> > Please keep our coding style in mind.
> > There should be no spaces after opening and before closing parenthesis.
>
> Do you have a perltidyrc config for your coding style?
Sorry, I don't use it and as far as I know the others don't either.
As far as I can tell there are just 2 main differences: we don't put
spaces after '(', and we use an admittedly somewhat unconventional
tab/space indentation mix (tabs = 8 spaces, but an indentation levels is
4 spaces, all groups of 8 spaces at the beginning of the line are tabs,
it's basically what you mostly get in vim with `:set ts=8 sts=4 sw=4 noet`)
(Though you might find the occasional style break in the existing code,
too, if something slipped through via patches. Longer patch sets
increase the chances of getting caught ;-P)
> > (Another one inline)
> >
> > On Mon, Feb 15, 2016 at 02:33:47PM +0100, Timo Grodzinski wrote:
> >> Signed-off-by: Timo Grodzinski <t.grodzin...@profihost.ag>
> >> ---
> >> PVE/QemuServer.pm | 20 ++++++++++++++++++++
> >> 1 file changed, 20 insertions(+)
> >>
> >> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> >> index a130596..6fafa59 100644
> >> --- a/PVE/QemuServer.pm
> >> +++ b/PVE/QemuServer.pm
> >> @@ -6547,6 +6547,26 @@ sub clone_disk {
> >> return $disk;
> >> }
> >>
> >> +sub get_image_size {
> >> + my ( $storecfg, $volid ) = @_;
> >> +
> >> + my $path = PVE::Storage::path( $storecfg, $volid );
> >> +
> >> + my @cmd = ( '/usr/bin/qemu-img', 'info', $path );
> >
> > We try to avoid having to use '\' to make references, so please use
> > + my $cmd = ['/usr/bin/qemu-img', 'info', $path];
>
> Why?
Mostly it makes reviewing later patches easier in case their
context-lines don't include a variable's declaration (since with that
rule we know to look more closely when we spot a $foo{bar} without a
'->' before the '{'.)
In this particular case the more important question is 'why not' ;-)
(Since the only use of @cmd is that single \@cmd line.)
> >
> >> + my $size;
> >> + my $parser = sub {
> >> + my ( $line ) = @_;
> >> + if ( $line =~ m/^virtual size: .*? \((\d+) bytes\)$/ ) {
> >> + $size = $1;
> >> + }
> >> + };
> >> +
> >> + eval { run_command( \@cmd, timeout => undef, outfunc => $parser ); };
> >
> > With the above change this is then simply $cmd.
> >
> >> + die "unable to get image size of volume id '$volid': $@" if $@;
> >> +
> >> + return $size;
> >> +}
> >> +
> >> # this only works if VM is running
> >> sub get_current_qemu_machine {
> >> my ($vmid) = @_;
> >
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
