Re: [pve-devel] [PATCH qemu-server] fix backpu/restore with ipv6/ports for pbs

[Dominik Csapak <d.csapak@proxmox.com>]

On 12/3/20 10:27 AM, Thomas Lamprecht wrote:
> On 03.12.20 09:43, Dominik Csapak wrote:
>> by copying the 'get_server_with_port' from PVE::Storage::PBSPlugin
>> here and using it for generating the server
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> ideally we would make the get_server_with_port sub in pve-storage
>> public and use it here (like we do e.g. in container with
>> 'run_raw_client_cmd') or even push that stuff into
> 
> we actually want to remove all run_raw_client_cmd usage, because it has no
> checks and is pretty raw - a clean interface would be much nicer..
> 

ok, makes sense

> 
> 
>> PVE::PBSClient module in comment to reuse that here and in pve-storage
>>
>> but to avoid the dependency bumps and fix it fast, this
>> should be enough for now
>>
> 
> I know I said to try to avoid the need for dependency bumps if possible, so
> this one is one me.

no problem :)

> 
>>   PVE/QemuServer.pm        | 14 +++++++++++++-
>>   PVE/VZDump/QemuServer.pm |  3 ++-
>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 4989938..7170230 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -6050,6 +6050,18 @@ sub rescan {
>>       }
>>   }
>>   
>> +sub pbs_get_server_with_port {
>> +    my ($scfg) = @_;
>> +
>> +    my $server = $scfg->{server};
>> +    $server = "[$server]" if $server =~ /^$IPV6RE$/;
>> +
>> +    if (my $port = $scfg->{port}) {
>> +	$server .= ":$port" if $port != 8007;
>> +    }
>> +    return $server;
>> +}
>> +
>>   sub restore_proxmox_backup_archive {
>>       my ($archive, $vmid, $user, $options) = @_;
>>   
>> @@ -6058,7 +6070,7 @@ sub restore_proxmox_backup_archive {
>>       my ($storeid, $volname) = PVE::Storage::parse_volume_id($archive);
>>       my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>>   
>> -    my $server = $scfg->{server};
>> +    my $server = pbs_get_server_with_port($scfg);
>>       my $datastore = $scfg->{datastore};
>>       my $username = $scfg->{username} // 'root@pam';
>>       my $fingerprint = $scfg->{fingerprint};
> 
> a few lines below, just out of context we see the sole use for all this extracting:
> 
> my $repo = "$username\@$server:$datastore";
> 
> So how about adding a get_repo sub to storage, or the PBSClient in pve-common
> (if we have a documented datastructure like scfg to get the info sanely from).

well afaics, we use everywhere the same section config options
(server,fingerprint,datastore,etc.) except there is no 'port'
setting in pmg (put this code here handles it ok, and we can add it no 
problem)
is that enough, or how else would you document it (or make
sure users know what to put in here)?

would a simple comment above the helper be enough? or should we
try to refactor those section config options across products?

so i'd simply add a 'build_repository' sub to pve-commons pbsclient module,
and use that everywhere we need that
is that ok?

> 
>> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
>> index 5003676..57436e6 100644
>> --- a/PVE/VZDump/QemuServer.pm
>> +++ b/PVE/VZDump/QemuServer.pm
>> @@ -473,7 +473,8 @@ sub archive_pbs {
>>   
>>       my $starttime = time();
>>   
>> -    my $server = $scfg->{server};
>> +    my $server = PVE::QemuServer::pbs_get_server_with_port($scfg);
>> +
>>       my $datastore = $scfg->{datastore};
>>       my $username = $scfg->{username} // 'root@pam';
>>       my $fingerprint = $scfg->{fingerprint};
>>
> 
>