[pve-devel] [PATCH qemu-server 2/4] add function returning virtual size of disk image

[Wolfgang Bumiller]

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.grodzinski at 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 at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>