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

[Mira Limbeck]

On 5/2/19 10:20 AM, Mira Limbeck wrote:
> On 4/30/19 3:14 PM, Thomas Lamprecht wrote:
>> 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..
> Ah yes, I assume it to always be either undefined (file does not exist 
> or other errors) or > 0. That's why I used !$size for the check. Could 
> be clarified, or the code changed to check for negative sizes as well 
> (if that makes any sense).

After going through the code another time, I now see that it always 
returns 0 or whatever 'qemu-img info' returns in the default 
implementation. (No plugin overrides the implementation)

So your check for $size <= 0 is the right one. Sorry about that.

>>> 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..
> Noted. But what if the implementation of file_size_info changes 
> without invalidating this use case? Then the commit message would no 
> longer be correct.
>>
>>> 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?
>
> Oh yes, sorry about that, with all the rearranging and code changes I 
> somehow missed the $d variable.
>
> Yes, setting it to 4MiB should be enough here (4 * 1024 * 1024 
> hopefully, not just 4 * 1024?), as the generated iso image should 
> always be < our disk image (file_get_contents limit set for custom 
> config files). The second file_size_info is unneccesary then.
>
>>
>> (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,
>>>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel