* [pve-devel] [PATCH qemu-server] fix backpu/restore with ipv6/ports for pbs
@ 2020-12-03 8:43 Dominik Csapak
2020-12-03 9:27 ` Thomas Lamprecht
0 siblings, 1 reply; 4+ messages in thread
From: Dominik Csapak @ 2020-12-03 8:43 UTC (permalink / raw)
To: pve-devel
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
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
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};
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};
--
2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] fix backpu/restore with ipv6/ports for pbs
2020-12-03 8:43 [pve-devel] [PATCH qemu-server] fix backpu/restore with ipv6/ports for pbs Dominik Csapak
@ 2020-12-03 9:27 ` Thomas Lamprecht
2020-12-03 9:40 ` Dominik Csapak
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Lamprecht @ 2020-12-03 9:27 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
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..
> 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.
> 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).
> 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};
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] fix backpu/restore with ipv6/ports for pbs
2020-12-03 9:27 ` Thomas Lamprecht
@ 2020-12-03 9:40 ` Dominik Csapak
2020-12-03 9:46 ` Thomas Lamprecht
0 siblings, 1 reply; 4+ messages in thread
From: Dominik Csapak @ 2020-12-03 9:40 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
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};
>>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] fix backpu/restore with ipv6/ports for pbs
2020-12-03 9:40 ` Dominik Csapak
@ 2020-12-03 9:46 ` Thomas Lamprecht
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2020-12-03 9:46 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
On 03.12.20 10:40, Dominik Csapak wrote:
>>
>> 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?
yeah a short comment and maybe the structure of known keys in the $cfg
variable should be enough for us - I do not see this changing soon.
I did something like that for the pve-storage "scan_datastores" sub:
https://git.proxmox.com/?p=pve-storage.git;a=commitdiff;h=8b62ac6a0ce0d4757ce25082123d18434f5b3a58
>
> so i'd simply add a 'build_repository' sub to pve-commons pbsclient module,
> and use that everywhere we need that
> is that ok?
Personally I'd went for "get_repository", build sounds a bit strange to me,
albeit it is not wrong (this has big bike shedding potential, so no hard
feelings from me).
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-03 9:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-03 8:43 [pve-devel] [PATCH qemu-server] fix backpu/restore with ipv6/ports for pbs Dominik Csapak
2020-12-03 9:27 ` Thomas Lamprecht
2020-12-03 9:40 ` Dominik Csapak
2020-12-03 9:46 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox