* [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