[pve-devel] applied: [PATCH v2 qemu-server] fix #2173: use file_size_info to check existence

[Thomas Lamprecht]

initially just wanted to review but to finaly move this forward: applied
comments still inline, can you please answer the question about the
second file info call? because I just threw a few followup on top, and
it would be great to verify that I did not missed anything ;-)

Am 4/30/19 um 2:20 PM schrieb Mira Limbeck:
> use file_size_info to check for existence of cloudinit disk instead of
> '-e'. this should solve the problem with rbd where the path returned by
> PVE::Storage::path is not checkable with '-e'. Any size > 0 is
> interpreted as the image existing.

you write any > 0 is seen as exist, but your code does any size != 0,
try to match this, else it just adds confusion and makes one question
the proposed solution more..

> 
> Signed-off-by: Mira Limbeck <m.limbeck at proxmox.com>
> ---
> v2:
>  - switched to file_size_info from list_images as it is by far less
>    heavy on large storages.

Do you mean in comparison to your previous approach of querying a list
of all images and grepping through it? I would have then rather something
like "we queried the size anyway, in the best case (existing image)
we have zero penalty, and worst case query it twice".

Also it could be useful to state a bit more directly that this is just
a "qemu-img info" command, maybe in the commit message..

> this was recommended by @Dominik instead of
>    the 'map_volume' solution discussed previously as we don't have to
>    map/unmap manually as qemu-img can work with 'rbd:...' paths.
> 
> The code still matches on (qcow2|raw) even though it was fixed in
> PVE/API2/Qemu.pm create_disks (bug 1829). Will fix that together with
> the restore fix.
> 
>  PVE/QemuServer/Cloudinit.pm | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
> index 445c777..bda48f1 100644
> --- a/PVE/QemuServer/Cloudinit.pm
> +++ b/PVE/QemuServer/Cloudinit.pm
> @@ -31,17 +31,18 @@ sub commit_cloudinit_disk {
>      my $iso_path = PVE::Storage::path($storecfg, $drive->{file});
>      my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>      my $format = PVE::QemuServer::qemu_img_format($scfg, $volname);
> -    if (! -e $iso_path) {
> +
> +    my $size = eval { PVE::Storage::file_size_info($iso_path) };
> +    if (!$size) {
>  	$volname =~ m/(vm-$vmid-cloudinit(.(qcow2|raw))?)/;
>  	my $name = $1;
>  	my $d = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $name, 4 * 1024);
> +	$size = PVE::Storage::file_size_info($iso_path);

wouldn't it be enough to set to 4 * 1024 (or whatever unit it is)?
Or is this to ensure we have the correct one if a storage pads/aligns
it up? Even then the 4MB would be enough for us here to pass to
qemu-img dd?

btw: isn't the $d variable unused here?

(I followed up with commits doing that, IMO we're actually on the safer
side using the 4MB for new disks, as the storage always can give us
something bigger than requested, but not smaller, and so using 4MB for
qemu-img dd ensure that a storage type can use more than 4MB (and thus
it works on one, but not the other, which would give a strange bug
report ^^)

>      }
>  
>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>      $plugin->activate_volume($storeid, $scfg, $volname);
>  
> -    my $size = PVE::Storage::file_size_info($iso_path);
> -
>      eval {
>  	run_command([['genisoimage', '-R', '-V', $label, $path],
>  		     ['qemu-img', 'dd', '-n', '-f', 'raw', '-O', $format,
>