From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with UTF8SMTPS id 1C17E61938 for ; Thu, 3 Dec 2020 10:40:46 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with UTF8SMTP id 14AB226B56 for ; Thu, 3 Dec 2020 10:40:46 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with UTF8SMTPS id 7F44726B46 for ; Thu, 3 Dec 2020 10:40:45 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with UTF8SMTP id 48EB6448B3; Thu, 3 Dec 2020 10:40:45 +0100 (CET) To: Thomas Lamprecht , Proxmox VE development discussion References: <20201203084309.24838-1-d.csapak@proxmox.com> From: Dominik Csapak Message-ID: <27fff7eb-5f72-b466-9409-94dc70b550da@proxmox.com> Date: Thu, 3 Dec 2020 10:40:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:84.0) Gecko/20100101 Thunderbird/84.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.298 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemuserver.pm] Subject: Re: [pve-devel] [PATCH qemu-server] fix backpu/restore with ipv6/ports for pbs X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Dec 2020 09:40:46 -0000 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 >> --- >> 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}; >> > >