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

[Thomas Lamprecht]

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).

Please check the implementation if you're unsure, we have the big advantage
of being able to relatively quickly check this, while it's not always clear,
here it would've been. But, for me that no "biggy", I just do not like if code
and commit message "do" different things. :)

>>> 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.

the commit message has a time stamp and _must_ seen with it in context.
You shouldn't try to start making your commit message future proof, as
_everything_ can change there, or do you have a working crystal ball? ;)

You can always hint at such things also, e.g.: "currently, file_size_info
uses just qemu-img info which handles all the details (rbd connection, ...)"


>>> +    $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.

OK, so my followup should be good, thanks for clarifying!