Re: [pve-devel] [PATCH storage 2/3] status: add new-filename to upload

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 20.07.21 15:27, Thomas Lamprecht wrote:
> On 20.07.21 13:51, Lorenz Stechauner wrote:
>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>> ---
>>  PVE/API2/Storage/Status.pm | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
>> index b549d7d..eac5e13 100644
>> --- a/PVE/API2/Storage/Status.pm
>> +++ b/PVE/API2/Storage/Status.pm
>> @@ -378,9 +378,15 @@ __PACKAGE__->register_method ({
>>  	    content => {
>>  		description => "Content type.",
>>  		type => 'string', format => 'pve-storage-content',
>> +		enum => ['iso', 'vztmpl'],
> 
> unrelated change? That could be send as its own patch, does not even needs to be a
> part of this series.
> 
>>  	    },
>>  	    filename => {
>> -		description => "The name of the file to create.",
>> +		description => "The original name of the file.",
>> +		type => 'string',
>> +	    },
>> +	    'new-filename' => {
>> +		description => "The name of the file to create. Caution: This will be normalized!",
>> +		maxLength => 255,
> 
> new non optional API parameter would be an ABI break, 7.0 is released, that won't
> fly anymore for ~1.9 years ;-)
> 
> Rather, make it optional and fallback to the filename (or whatever makes it
> actually backward compatible).

Intially I just saw the new non-optional param and ABI break was enough to comment
that, but after I took another glance here the new param seems rather bogus in general?

So, what's it actual use? Why is the existing `filename` one not enough?
It'd be good if such things would be stated in the commit message already..

> 
>>  		type => 'string',
>>  	    },
>>  	    tmpfilename => {
>> @@ -414,7 +420,7 @@ __PACKAGE__->register_method ({
>>  	my $size = -s $tmpfilename;
>>  	die "temporary file '$tmpfilename' does not exist\n" if !defined($size);
>>  
>> -	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
>> +	my $filename = PVE::Storage::normalize_content_filename($param->{'new-filename'});
>>  
>>  	my $path;
>>  
>>
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>