[pve-devel] [qemu-server] fix #1829 check cloud-init disk format.

[Thomas Lamprecht]

On 7/11/18 12:06 PM, Wolfgang Bumiller wrote:
> On Tue, Jul 03, 2018 at 05:05:54PM +0200, Wolfgang Link wrote:
>> Directory storage support raw and qcow2 format.
>> It is not good to implicitly set the cloud-init format
>> and ignore the selected format.
>>
>> Signed-off-by: Wolfgang Link <w.link at proxmox.com>
>> ---
>>  PVE/API2/Qemu.pm | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index c15c71f..26ace09 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -149,12 +149,14 @@ my $create_disks = sub {
>>  	    die "no storage ID specified (and no default storage)\n" if !$storeid;
>>  	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>>  	    my $name = "vm-$vmid-cloudinit";
>> -	    my $fmt = undef;
>> +	    my $fmt = $disk->{format};
>>  	    if ($scfg->{path}) {
>> -		$name .= ".qcow2";
>> -		$fmt = 'qcow2';
>> +		die "Format \"$fmt\" is not supported\n"
>> +		    if $fmt !~ m/[raw|qcow2]/;
> 
> Wrong regex.
> 
> IIRC we said that we eg. don't want vmdk here, so that's why you're
> doing this, but I find this is unnecessary work... Can we just skip the
> check?
> 
>> +		$name .= ".$fmt";
> 
> BTW. @Thomas, @Dietmar:
> Why do we even need to add the extension? This is a storage-dependent
> thing, vdisk_alloc() currently actually verifies that the extension is
> there for storages which need it, but instead, it could just add it,
> because here we need to needlessly distinguish between storage types for
> a function which returns the actually allocated volid anyway.
> 
> We could make the extension optional and simplify code like this...
>  => QemuConfig.pm
>     > $name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
>  => API2/Storage/Content.pm
>     `create` takes a 'format' option and a filename, and verifies that
>     the extension matches the option if both exist.
>     (Weirdly the parameter is called `filename` actually...)
>  => QemuServer.pm
>     clone_disk has a cloud-init case where it appends .qcow2 (which btw.
>     I think will fail with .raw as well currently...)
> 

That sounds good to me, the pass format and name, then extract format from
name and check if they're the same seems really a bit weird.

> 
>>  	    }else{
>> -		$fmt = 'raw';
>> +		die "Format \"$fmt\" is not supported\n"
>> +		    if $fmt eq 'raw';
>>  	    }
>>  	    # FIXME: Reasonable size? qcow2 shouldn't grow if the space isn't used anyway?
>>  	    my $cloudinit_iso_size = 5; # in MB
>> -- 
>> 2.11.0